Add digest delivery retries and observability metrics
swadhinbiswas committed 8 months ago
4b685b4
1 parent d1172bd
commit 4b685b4
8 files changed
+235 -18
Showing 8 changed files+235-18
▼doc/feature_audit.md
+1-1
| @@ -184,7 +184,7 @@ | ||
184 | 184 | | :--- | :---: | :--- | |
185 | 185 | | Smart notifications | ⚠️ | API now supports smart priority scoring/sorting via `GET /notifications?prioritize=true`; deeper ML personalization and channel-level routing remain partial | |
186 | 186 | | Blocking alerts | ⚠️ | Blocking-focused notification feed filter is now available via `GET /notifications?filter=blocking`; prioritization/automation depth remains partial | |
187 | -| Daily/weekly digests | ⚠️ | Scheduler is timezone-aware with cron endpoint/tests and user-scoped digest test API (`/user/notification-digests/test`); delivery observability/retry depth remains partial | | |
187 | +| Daily/weekly digests | ⚠️ | Scheduler is timezone-aware with cron endpoint/tests, user-scoped digest test API (`/user/notification-digests/test`), and bounded retry/observability metrics (`maxRetries`, `retried`, `recovered`); provider-level delivery analytics and dead-letter tooling remain partial | | |
188 | 188 | | Mentions & subscriptions | ✅ | Implemented | |
189 | 189 | | PR-level discussions | ✅ | Full threaded comments | |
190 | 190 | | Activity feeds | ✅ | Dashboard activity feed | |
191 | 191 | |
▼src/lib/chat-notifications.ts
+98-14
| @@ -765,6 +765,7 @@ export function shouldSendDigestNow( | ||
765 | 765 | export interface DigestRunOptions { |
766 | 766 | now?: Date; |
767 | 767 | dryRun?: boolean; |
768 | + maxRetries?: number; | |
768 | 769 | } |
769 | 770 | |
770 | 771 | export interface DigestRunResult { |
| @@ -774,6 +775,8 @@ export interface DigestRunResult { | ||
774 | 775 | skippedNoEmail: number; |
775 | 776 | skippedEmpty: number; |
776 | 777 | failed: number; |
778 | + retried: number; | |
779 | + recovered: number; | |
777 | 780 | } |
778 | 781 | |
779 | 782 | export interface UserDigestRunOptions { |
| @@ -781,6 +784,7 @@ export interface UserDigestRunOptions { | ||
781 | 784 | period?: "daily" | "weekly"; |
782 | 785 | now?: Date; |
783 | 786 | dryRun?: boolean; |
787 | + maxRetries?: number; | |
784 | 788 | } |
785 | 789 | |
786 | 790 | export interface UserDigestRunResult { |
| @@ -789,12 +793,62 @@ export interface UserDigestRunResult { | ||
789 | 793 | period: "daily" | "weekly"; |
790 | 794 | itemCount: number; |
791 | 795 | reason?: "missing_user_or_email" | "empty_digest" | "send_failed" | "missing_digest_settings"; |
796 | + attempts: number; | |
797 | + recovered?: boolean; | |
798 | + lastError?: string; | |
799 | +} | |
800 | + | |
801 | +function normalizeRetryCount(value: number | undefined): number { | |
802 | + if (typeof value !== "number" || Number.isNaN(value)) return 1; | |
803 | + return Math.min(Math.max(Math.trunc(value), 0), 5); | |
804 | +} | |
805 | + | |
806 | +export interface DigestDeliveryAttemptResult { | |
807 | + sent: boolean; | |
808 | + attempts: number; | |
809 | + recovered: boolean; | |
810 | + lastError?: string; | |
811 | +} | |
812 | + | |
813 | +export async function sendDigestWithRetry( | |
814 | + send: () => Promise<boolean>, | |
815 | + maxRetries: number | |
816 | +): Promise<DigestDeliveryAttemptResult> { | |
817 | + const retries = normalizeRetryCount(maxRetries); | |
818 | + const maxAttempts = retries + 1; | |
819 | + let attempts = 0; | |
820 | + let lastError: string | undefined; | |
821 | + | |
822 | + while (attempts < maxAttempts) { | |
823 | + attempts++; | |
824 | + try { | |
825 | + const sent = await send(); | |
826 | + if (sent) { | |
827 | + return { | |
828 | + sent: true, | |
829 | + attempts, | |
830 | + recovered: attempts > 1, | |
831 | + }; | |
832 | + } | |
833 | + lastError = "send_failed"; | |
834 | + } catch (error) { | |
835 | + lastError = error instanceof Error ? error.message : "send_failed"; | |
836 | + } | |
837 | + } | |
838 | + | |
839 | + return { | |
840 | + sent: false, | |
841 | + attempts, | |
842 | + recovered: false, | |
843 | + lastError, | |
844 | + }; | |
792 | 845 | } |
793 | 846 | |
794 | 847 | export async function runUserDigest(options: UserDigestRunOptions): Promise<UserDigestRunResult> { |
795 | 848 | const db = getDatabase() as NodePgDatabase<typeof schema>; |
796 | 849 | const now = options.now ?? new Date(); |
797 | 850 | const dryRun = options.dryRun ?? true; |
851 | + const maxRetries = normalizeRetryCount(options.maxRetries); | |
798 | 852 | |
799 | 853 | const setting = await db.query.emailDigestSettings.findFirst({ |
800 | 854 | where: eq(schema.emailDigestSettings.userId, options.userId), |
| @@ -806,6 +860,7 @@ export async function runUserDigest(options: UserDigestRunOptions): Promise<User | ||
806 | 860 | period: options.period || "daily", |
807 | 861 | itemCount: 0, |
808 | 862 | reason: "missing_digest_settings", |
863 | + attempts: 0, | |
809 | 864 | }; |
810 | 865 | } |
811 | 866 | |
| @@ -821,6 +876,7 @@ export async function runUserDigest(options: UserDigestRunOptions): Promise<User | ||
821 | 876 | period, |
822 | 877 | itemCount: 0, |
823 | 878 | reason: "missing_user_or_email", |
879 | + attempts: 0, | |
824 | 880 | }; |
825 | 881 | } |
826 | 882 | |
| @@ -846,25 +902,34 @@ export async function runUserDigest(options: UserDigestRunOptions): Promise<User | ||
846 | 902 | period, |
847 | 903 | itemCount: 0, |
848 | 904 | reason: "empty_digest", |
905 | + attempts: 0, | |
849 | 906 | }; |
850 | 907 | } |
851 | 908 | |
909 | + let attempts = 0; | |
910 | + let recovered = false; | |
852 | 911 | if (!dryRun) { |
853 | - const sent = await sendPlatformEmail({ | |
854 | - to: user.email, | |
855 | - subject: digest.subject, | |
856 | - html: digest.html, | |
857 | - text: `${digest.subject}\n\nYou have ${digest.itemCount} new notification(s).`, | |
858 | - }); | |
859 | - if (!sent) { | |
912 | + const delivery = await sendDigestWithRetry(async () => { | |
913 | + return sendPlatformEmail({ | |
914 | + to: user.email, | |
915 | + subject: digest.subject, | |
916 | + html: digest.html, | |
917 | + text: `${digest.subject}\n\nYou have ${digest.itemCount} new notification(s).`, | |
918 | + }); | |
919 | + }, maxRetries); | |
920 | + if (!delivery.sent) { | |
860 | 921 | return { |
861 | 922 | sent: false, |
862 | 923 | dryRun, |
863 | 924 | period, |
864 | 925 | itemCount: digest.itemCount, |
865 | 926 | reason: "send_failed", |
927 | + attempts: delivery.attempts, | |
928 | + lastError: delivery.lastError, | |
866 | 929 | }; |
867 | 930 | } |
931 | + attempts = delivery.attempts; | |
932 | + recovered = delivery.recovered; | |
868 | 933 | |
869 | 934 | await db |
870 | 935 | .update(schema.emailDigestSettings) |
| @@ -877,6 +942,8 @@ export async function runUserDigest(options: UserDigestRunOptions): Promise<User | ||
877 | 942 | dryRun, |
878 | 943 | period, |
879 | 944 | itemCount: digest.itemCount, |
945 | + attempts: dryRun ? 0 : attempts, | |
946 | + recovered, | |
880 | 947 | }; |
881 | 948 | } |
882 | 949 | |
| @@ -884,6 +951,7 @@ export async function runDueDigests(options: DigestRunOptions = {}): Promise<Dig | ||
884 | 951 | const db = getDatabase() as NodePgDatabase<typeof schema>; |
885 | 952 | const now = options.now ?? new Date(); |
886 | 953 | const dryRun = options.dryRun ?? false; |
954 | + const maxRetries = normalizeRetryCount(options.maxRetries); | |
887 | 955 | |
888 | 956 | const result: DigestRunResult = { |
889 | 957 | checked: 0, |
| @@ -892,6 +960,8 @@ export async function runDueDigests(options: DigestRunOptions = {}): Promise<Dig | ||
892 | 960 | skippedNoEmail: 0, |
893 | 961 | skippedEmpty: 0, |
894 | 962 | failed: 0, |
963 | + retried: 0, | |
964 | + recovered: 0, | |
895 | 965 | }; |
896 | 966 | |
897 | 967 | const settings = await db.query.emailDigestSettings.findMany({ |
| @@ -960,16 +1030,30 @@ export async function runDueDigests(options: DigestRunOptions = {}): Promise<Dig | ||
960 | 1030 | } |
961 | 1031 | |
962 | 1032 | if (!dryRun) { |
963 | - const sent = await sendPlatformEmail({ | |
964 | - to: user.email, | |
965 | - subject: digest.subject, | |
966 | - html: digest.html, | |
967 | - text: `${digest.subject}\n\nYou have ${digest.itemCount} new notification(s).`, | |
968 | - }); | |
969 | - if (!sent) { | |
1033 | + const delivery = await sendDigestWithRetry(async () => { | |
1034 | + return sendPlatformEmail({ | |
1035 | + to: user.email, | |
1036 | + subject: digest.subject, | |
1037 | + html: digest.html, | |
1038 | + text: `${digest.subject}\n\nYou have ${digest.itemCount} new notification(s).`, | |
1039 | + }); | |
1040 | + }, maxRetries); | |
1041 | + if (delivery.attempts > 1) { | |
1042 | + result.retried += delivery.attempts - 1; | |
1043 | + } | |
1044 | + if (!delivery.sent) { | |
970 | 1045 | result.failed++; |
1046 | + logger.error({ | |
1047 | + userId: setting.userId, | |
1048 | + digestSettingId: setting.id, | |
1049 | + attempts: delivery.attempts, | |
1050 | + lastError: delivery.lastError || "send_failed", | |
1051 | + }, "Digest send retries exhausted"); | |
971 | 1052 | continue; |
972 | 1053 | } |
1054 | + if (delivery.recovered) { | |
1055 | + result.recovered++; | |
1056 | + } | |
973 | 1057 | |
974 | 1058 | await db |
975 | 1059 | .update(schema.emailDigestSettings) |
976 | 1060 | |
▼src/lib/openapi.ts
+27-0
| @@ -269,6 +269,7 @@ export const openApiSpec = { | ||
269 | 269 | properties: { |
270 | 270 | dryRun: { type: "boolean", default: true }, |
271 | 271 | period: { type: "string", enum: ["daily", "weekly"] }, |
272 | + maxRetries: { type: "integer", minimum: 0, maximum: 5, default: 1 }, | |
272 | 273 | }, |
273 | 274 | }, |
274 | 275 | }, |
| @@ -936,6 +937,32 @@ export const openApiSpec = { | ||
936 | 937 | }, |
937 | 938 | }, |
938 | 939 | }, |
940 | + "/cron/notification-digests": { | |
941 | + post: { | |
942 | + tags: ["Notifications"], | |
943 | + summary: "Run scheduled digest delivery", | |
944 | + description: "Cron-authenticated endpoint to process due digests. Supports `dryRun=true` and bounded retry depth via `maxRetries`.", | |
945 | + parameters: [ | |
946 | + { | |
947 | + name: "dryRun", | |
948 | + in: "query", | |
949 | + required: false, | |
950 | + schema: { type: "boolean", default: false }, | |
951 | + }, | |
952 | + { | |
953 | + name: "maxRetries", | |
954 | + in: "query", | |
955 | + required: false, | |
956 | + schema: { type: "integer", minimum: 0, maximum: 5, default: 1 }, | |
957 | + }, | |
958 | + ], | |
959 | + responses: { | |
960 | + 200: { description: "Digest run completed" }, | |
961 | + 401: { description: "Unauthorized" }, | |
962 | + 500: { description: "Digest run failed" }, | |
963 | + }, | |
964 | + }, | |
965 | + }, | |
939 | 966 | "/notifications": { |
940 | 967 | get: { |
941 | 968 | tags: ["Notifications"], |
942 | 969 | |
▼src/pages/api/cron/notification-digests.ts
+49-0
| @@ -0,0 +1,49 @@ | ||
1 | +import type { APIRoute } from "astro"; | |
2 | +import { runDueDigests } from "@/lib/chat-notifications"; | |
3 | +import { logger } from "@/lib/logger"; | |
4 | + | |
5 | +const CRON_SECRET = process.env.CRON_SECRET; | |
6 | + | |
7 | +function isAuthorized(request: Request): boolean { | |
8 | + if (!CRON_SECRET) return false; | |
9 | + const authHeader = request.headers.get("Authorization"); | |
10 | + return authHeader === `Bearer ${CRON_SECRET}`; | |
11 | +} | |
12 | + | |
13 | +export const POST: APIRoute = async ({ request, url }) => { | |
14 | + if (!isAuthorized(request)) { | |
15 | + return new Response("Unauthorized", { status: 401 }); | |
16 | + } | |
17 | + | |
18 | + const dryRun = url.searchParams.get("dryRun") === "true"; | |
19 | + const rawMaxRetries = url.searchParams.get("maxRetries"); | |
20 | + const parsedMaxRetries = rawMaxRetries ? Number.parseInt(rawMaxRetries, 10) : 1; | |
21 | + const maxRetries = Number.isNaN(parsedMaxRetries) ? 1 : parsedMaxRetries; | |
22 | + | |
23 | + try { | |
24 | + const result = await runDueDigests({ dryRun, maxRetries }); | |
25 | + logger.info({ dryRun, maxRetries, ...result }, "Notification digest cron completed"); | |
26 | + | |
27 | + return new Response(JSON.stringify({ | |
28 | + success: true, | |
29 | + dryRun, | |
30 | + maxRetries, | |
31 | + ...result, | |
32 | + }), { | |
33 | + status: 200, | |
34 | + headers: { "Content-Type": "application/json" }, | |
35 | + }); | |
36 | + } catch (error) { | |
37 | + const message = error instanceof Error ? error.message : "Unknown error"; | |
38 | + logger.error({ error: message, dryRun, maxRetries }, "Notification digest cron failed"); | |
39 | + return new Response(JSON.stringify({ | |
40 | + success: false, | |
41 | + error: message, | |
42 | + }), { | |
43 | + status: 500, | |
44 | + headers: { "Content-Type": "application/json" }, | |
45 | + }); | |
46 | + } | |
47 | +}; | |
48 | + | |
49 | +export const GET = POST; | |
0 | 50 | |
▼src/pages/api/user/notification-digests/test.ts
+2-1
| @@ -7,6 +7,7 @@ import { runUserDigest } from "@/lib/chat-notifications"; | ||
7 | 7 | const testDigestSchema = z.object({ |
8 | 8 | dryRun: z.boolean().optional(), |
9 | 9 | period: z.enum(["daily", "weekly"]).optional(), |
10 | + maxRetries: z.number().int().min(0).max(5).optional(), | |
10 | 11 | }); |
11 | 12 | |
12 | 13 | export const POST: APIRoute = withErrorHandler(async ({ locals, request }) => { |
| @@ -23,8 +24,8 @@ export const POST: APIRoute = withErrorHandler(async ({ locals, request }) => { | ||
23 | 24 | userId: user.id, |
24 | 25 | dryRun: parsed.data.dryRun ?? true, |
25 | 26 | period: parsed.data.period, |
27 | + maxRetries: parsed.data.maxRetries, | |
26 | 28 | }); |
27 | 29 | |
28 | 30 | return success(result); |
29 | 31 | }); |
30 | - | |
31 | 32 | |
▼tests/integration/user-digest-test-route.test.ts
+3-1
| @@ -6,6 +6,7 @@ const { runUserDigestMock } = vi.hoisted(() => ({ | ||
6 | 6 | dryRun: true, |
7 | 7 | period: "daily", |
8 | 8 | itemCount: 3, |
9 | + attempts: 0, | |
9 | 10 | })), |
10 | 11 | })); |
11 | 12 | |
| @@ -26,6 +27,7 @@ describe("user digest test route", () => { | ||
26 | 27 | dryRun: true, |
27 | 28 | period: "daily", |
28 | 29 | itemCount: 3, |
30 | + attempts: 0, | |
29 | 31 | }); |
30 | 32 | }); |
31 | 33 | |
| @@ -57,7 +59,7 @@ describe("user digest test route", () => { | ||
57 | 59 | userId: "user-1", |
58 | 60 | dryRun: true, |
59 | 61 | period: undefined, |
62 | + maxRetries: undefined, | |
60 | 63 | }); |
61 | 64 | }); |
62 | 65 | }); |
63 | - | |
64 | 66 | |
▼tests/unit/digest-delivery-retry.test.ts
+25-0
| @@ -0,0 +1,25 @@ | ||
1 | +import { describe, expect, it, vi } from "vitest"; | |
2 | +import { sendDigestWithRetry } from "@/lib/chat-notifications"; | |
3 | + | |
4 | +describe("digest delivery retry helper", () => { | |
5 | + it("recovers after a retry when a later attempt succeeds", async () => { | |
6 | + const send = vi | |
7 | + .fn<() => Promise<boolean>>() | |
8 | + .mockResolvedValueOnce(false) | |
9 | + .mockResolvedValueOnce(true); | |
10 | + | |
11 | + const result = await sendDigestWithRetry(send, 2); | |
12 | + expect(result.sent).toBe(true); | |
13 | + expect(result.recovered).toBe(true); | |
14 | + expect(result.attempts).toBe(2); | |
15 | + }); | |
16 | + | |
17 | + it("returns failure after exhausting retries", async () => { | |
18 | + const send = vi.fn<() => Promise<boolean>>().mockResolvedValue(false); | |
19 | + | |
20 | + const result = await sendDigestWithRetry(send, 1); | |
21 | + expect(result.sent).toBe(false); | |
22 | + expect(result.attempts).toBe(2); | |
23 | + expect(result.lastError).toBe("send_failed"); | |
24 | + }); | |
25 | +}); | |
0 | 26 | |
▼tests/unit/notification-digest-cron.test.ts
+30-1
| @@ -81,10 +81,39 @@ describe("notification digest cron API", () => { | ||
81 | 81 | expect(response.status).toBe(200); |
82 | 82 | expect(body.success).toBe(true); |
83 | 83 | expect(body.dryRun).toBe(true); |
84 | - expect(runDueDigestsMock).toHaveBeenCalledWith({ dryRun: true }); | |
84 | + expect(body.maxRetries).toBe(1); | |
85 | + expect(runDueDigestsMock).toHaveBeenCalledWith({ dryRun: true, maxRetries: 1 }); | |
85 | 86 | expect(loggerInfoMock).toHaveBeenCalledTimes(1); |
86 | 87 | }); |
87 | 88 | |
89 | + it("passes maxRetries query param to digest runner", async () => { | |
90 | + vi.stubEnv("CRON_SECRET", "super-secret"); | |
91 | + runDueDigestsMock.mockResolvedValue({ | |
92 | + checked: 1, | |
93 | + due: 1, | |
94 | + sent: 1, | |
95 | + skippedNoEmail: 0, | |
96 | + skippedEmpty: 0, | |
97 | + failed: 0, | |
98 | + retried: 1, | |
99 | + recovered: 1, | |
100 | + }); | |
101 | + vi.resetModules(); | |
102 | + const mod = await import("@/pages/api/cron/notification-digests"); | |
103 | + | |
104 | + const request = new Request("http://localhost/api/cron/notification-digests?maxRetries=3", { | |
105 | + method: "POST", | |
106 | + headers: { Authorization: "Bearer super-secret" }, | |
107 | + }); | |
108 | + | |
109 | + const response = await mod.POST({ request, url: new URL(request.url) } as any); | |
110 | + const body = await response.json(); | |
111 | + | |
112 | + expect(response.status).toBe(200); | |
113 | + expect(body.maxRetries).toBe(3); | |
114 | + expect(runDueDigestsMock).toHaveBeenCalledWith({ dryRun: false, maxRetries: 3 }); | |
115 | + }); | |
116 | + | |
88 | 117 | it("returns 500 when digest runner throws", async () => { |
89 | 118 | vi.stubEnv("CRON_SECRET", "super-secret"); |
90 | 119 | runDueDigestsMock.mockRejectedValue(new Error("boom")); |
91 | 120 | |