From 5ef87f973a68c5e09f139fb32a58ec8f59ce6357 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 16 Oct 2026 01:35:36 -0700 Subject: [PATCH] fix(digest): a switch read error mid-run is a blip, not a flip sprint-review on #2277 (non-blocking): the switch helper failed closed on a read error, and inside the loop that was indistinguishable from a deliberate flip: both stopped the run. A summary skipped by a stop is never retried (each run sends only its own), so a transient read error mid-run cost every remaining recipient that day's digest. The loss mode existed at the run-start read; reading per recipient multiplied the chances to hit it. The helper now says on, off or unknown. At the start of a run, anything but on sends nothing (unchanged: the switch fails closed when nothing is known). Mid-run the switch is already known to have been on, so only a definite off stops the run, and a read error lets it continue. The runbook says so. Tests: a flip between the run's first read and the first send stops that email too (the read sits before the send); a read error mid-run sends the rest. Mutations: the mid-run read stopping on unknown, the run-start read failing open, and the per-recipient read moved after the send each red an arm. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0157AG2wo8mJYqKezXNs48Ya --- .../unit/services/digestEmailService.test.js | 28 +++++++++++++++++++ backend/services/digestEmailService.ts | 24 ++++++++++------ docs/runbooks/daily-digest.md | 5 +++- 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/backend/__tests__/unit/services/digestEmailService.test.js b/backend/__tests__/unit/services/digestEmailService.test.js index 941a9485a6..cb54585661 100644 --- a/backend/__tests__/unit/services/digestEmailService.test.js +++ b/backend/__tests__/unit/services/digestEmailService.test.js @@ -228,6 +228,34 @@ describe('digestEmailService', () => { expect(mockSendEmail).toHaveBeenCalledTimes(1); }); + // sprint-review on #2277: the per-recipient read sits BEFORE the send, so a + // flip between the run's first read and the first send stops that email too. + it('a flip before the first send stops even the first email', async () => { + const user = await createUser(); + const digest = await createDigest(user); + const read = jest.spyOn(SystemSetting, 'findOne') + .mockImplementationOnce(() => ({ lean: async () => null })) + .mockImplementationOnce(() => ({ lean: async () => ({ value: { enabled: false } }) })); + + await expect(digestEmailService.sendDigestEmails([runEntry(digest)])) + .resolves.toEqual(expect.objectContaining({ sent: 0, disabled: true })); + expect(mockSendEmail).not.toHaveBeenCalled(); + read.mockRestore(); + }); + + // ...and an unreadable switch mid-run is a read blip, not a flip: the switch + // was on at the start, and a summary skipped here is never retried. + it('a read error mid-run does not stop the rest of the run', async () => { + const digests = [await createDigest(await createUser()), await createDigest(await createUser())]; + const read = jest.spyOn(SystemSetting, 'findOne') + .mockImplementationOnce(() => ({ lean: async () => null })) + .mockImplementation(() => { throw new Error('mongo blip'); }); + + await expect(digestEmailService.sendDigestEmails(digests.map(runEntry))) + .resolves.toEqual(expect.objectContaining({ sent: 2, disabled: false })); + read.mockRestore(); + }); + it('sends nothing when the switch cannot be read', async () => { const user = await createUser(); const digest = await createDigest(user); diff --git a/backend/services/digestEmailService.ts b/backend/services/digestEmailService.ts index 1a3c352ffa..c511c2d3e2 100644 --- a/backend/services/digestEmailService.ts +++ b/backend/services/digestEmailService.ts @@ -18,15 +18,20 @@ const SystemSetting = require('../models/SystemSetting'); */ export const DIGEST_SENDING_KEY = 'digest.sending'; -// Unreadable means do not send: for email to people, the switch fails closed. -const digestSendingEnabled = async (): Promise => { +// 'unknown' is a read that failed, which is not the same as a flip: at the +// start of a run it means do not send (for email to people the switch fails +// closed), but mid-run the switch is already known to have been on, so only a +// definite 'off' stops the run. A summary skipped by a stop is never retried +// (each run sends only its own), so a read blip must not forfeit the rest. +type DigestSendingState = 'on' | 'off' | 'unknown'; +const digestSendingState = async (): Promise => { try { const row = await SystemSetting.findOne({ key: DIGEST_SENDING_KEY }) .lean() as { value?: { enabled?: unknown } } | null; - return row?.value?.enabled !== false; + return row?.value?.enabled === false ? 'off' : 'on'; } catch (error) { - console.warn('Digest email delivery skipped: the sending switch could not be read:', (error as Error).message); - return false; + console.warn('Digest email: the sending switch could not be read:', (error as Error).message); + return 'unknown'; } }; @@ -132,8 +137,11 @@ export class DigestEmailService { disabled: false, }; - if (!await digestSendingEnabled()) { - console.warn(`Digest email delivery skipped: switched off (${DIGEST_SENDING_KEY}).`); + const atStart = await digestSendingState(); + if (atStart !== 'on') { + console.warn(atStart === 'off' + ? `Digest email delivery skipped: switched off (${DIGEST_SENDING_KEY}).` + : `Digest email delivery skipped: the sending switch is unreadable (${DIGEST_SENDING_KEY}).`); result.disabled = true; return result; } @@ -161,7 +169,7 @@ export class DigestEmailService { // Read again per recipient (sprint-review on #2275): the switch is for an // incident, and an incident is often a run that is already sending. // eslint-disable-next-line no-await-in-loop - if (!await digestSendingEnabled()) { + if (await digestSendingState() === 'off') { console.warn(`Digest email delivery stopped mid-run: switched off (${DIGEST_SENDING_KEY}).`); result.disabled = true; break; diff --git a/docs/runbooks/daily-digest.md b/docs/runbooks/daily-digest.md index 455e08388f..5c38f6b23c 100644 --- a/docs/runbooks/daily-digest.md +++ b/docs/runbooks/daily-digest.md @@ -30,7 +30,10 @@ m.connect(process.env.MONGO_URI).then(async () => { ``` To turn it back on, run the same command with `enabled: true`, or delete the -row. If the row can't be read, nothing is sent: the switch fails closed. A run +row. If the row can't be read when a run starts, nothing is sent: the switch +fails closed. A read error partway through a run that started switched on is +treated as a blip, not a flip, so it doesn't stop the run; only a definite +`enabled: false` does. A run logs `Digest email delivery skipped: switched off (digest.sending).` when it starts switched off, and `Digest email delivery stopped mid-run: ...` when the switch is flipped while it's sending.