From fdf8eece6256118fdae7210fc687625d8d442801 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:47:20 +0100 Subject: [PATCH] fix: keep a rescheduled send from going out twice or not at all when the server's answer is lost --- src/api/__tests__/email-submission.test.ts | 90 +++++++++++++++++++ .../__tests__/live-stalwart-compose.test.ts | 63 ++++++++++++- src/api/email.ts | 71 +++++++++++++-- 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/src/api/__tests__/email-submission.test.ts b/src/api/__tests__/email-submission.test.ts index e148012a..f7caea65 100644 --- a/src/api/__tests__/email-submission.test.ts +++ b/src/api/__tests__/email-submission.test.ts @@ -218,6 +218,96 @@ describe('rescheduleScheduledSend', () => { vi.mocked(Date.now).mockRestore(); }); + describe('answers that never came', () => { + const status = (undoStatus: string) => ({ + methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus }] }, '0']], + }); + const records = (list: Array>) => ({ + methodResponses: [ + ['EmailSubmission/query', { ids: list.map((s) => s.id) }, '0'], + ['EmailSubmission/get', { list }, '1'], + ], + }); + const creates = () => setCalls().filter(([created]) => created.length > 0).length; + + it('goes on to the replacement when the cancel went through but its answer was lost', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce(status('canceled')) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); + + const result = await rescheduleScheduledSend(SCHEDULED, 0); + + expect(result.emailSubmissionId).toBe('sub-2'); + // The canceled record goes, as after a confirmed cancel. + expect(setCalls()).toEqual([[[], ['sub-1'], []], [[], [], ['sub-1']], [['replacement'], [], []]]); + }); + + it('still makes no replacement when the server refused the cancel and the original was cancelled elsewhere', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(set({ notUpdated: { 'sub-1': { type: 'invalidProperties' } } })) + .mockResolvedValueOnce(status('canceled')); + + const err = await rescheduleScheduledSend(SCHEDULED, 0).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(NoLongerScheduledError); + expect(creates()).toBe(0); + }); + + it('takes a replacement the server made for the answer that was lost, and does not put the original back', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce(records([ + { id: 'sub-1', emailId: 'e-1', undoStatus: 'canceled' }, + { id: 'sub-2', emailId: 'e-1', undoStatus: 'final', sendAt: '2026-09-25T08:00:00Z' }, + ])); + + const result = await rescheduleScheduledSend(SCHEDULED, 0); + + expect(result).toEqual({ emailSubmissionId: 'sub-2', sendAt: '2026-09-25T08:00:00Z' }); + expect(creates()).toBe(1); + const [query] = mockRequest.mock.calls.at(-1)![0]; + expect(query).toEqual(['EmailSubmission/query', { accountId: 'acc-1', filter: { emailIds: ['e-1'] } }, '0']); + }); + + it('puts the original back when the lost create left nothing on the server', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockRejectedValueOnce(new Error('Request timed out')) + .mockResolvedValueOnce(records([{ id: 'sub-1', emailId: 'e-1', undoStatus: 'canceled' }])) + .mockResolvedValueOnce(set({ created: { replacement: { id: 'sub-3' } } })); + + const err = await rescheduleScheduledSend(SCHEDULED, 7200).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(RescheduleRestoredError); + expect(err).toMatchObject({ restoredSubmissionId: 'sub-3' }); + }); + + it('counts a put-back the server made for the answer that was lost as restored, and leaves the message in Sent', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce(REFUSED('Quota exceeded')) + .mockRejectedValueOnce(new Error('Network request failed')) + .mockResolvedValueOnce(records([{ id: 'sub-3', emailId: 'e-1', undoStatus: 'pending', sendAt: ORIGINAL_AT }])); + + const err = await rescheduleScheduledSend(SCHEDULED, 7200).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(RescheduleRestoredError); + expect(err).toMatchObject({ restoredSubmissionId: 'sub-3', restoredSendAt: ORIGINAL_AT }); + expect(mockRequest.mock.calls.some((c) => c[0][0][0] === 'Email/set')).toBe(false); + }); + }); + // A group send's undo entry carried the login's own identity id, which // names nothing in the group account: both creates were refused and the // message went back to Drafts unsent. diff --git a/src/api/__tests__/live-stalwart-compose.test.ts b/src/api/__tests__/live-stalwart-compose.test.ts index a8ba70d3..5b2b922e 100644 --- a/src/api/__tests__/live-stalwart-compose.test.ts +++ b/src/api/__tests__/live-stalwart-compose.test.ts @@ -22,7 +22,7 @@ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { JMAPClient } from '../jmap-client'; import { - destroyEmails, getFullEmails, getMailboxes, listScheduledEmails, rescheduleScheduledSend, saveDraftVersion, sendEmail, + cancelScheduledSend, destroyEmails, getFullEmails, getMailboxes, listScheduledEmails, rescheduleScheduledSend, saveDraftVersion, sendEmail, } from '../email'; import { uploadBytes } from '../blob'; import { deleteFileNodes } from '../files'; @@ -69,6 +69,11 @@ live('live Stalwart: composer and sending (0.16 and 1.0)', () => { }, 60_000); afterAll(async () => { + // A failed test may leave a send scheduled; it must not outlive its message. + const held = await listScheduledEmails().catch(() => []); + for (const s of held.filter((h) => createdEmails.includes(h.emailId))) { + await cancelScheduledSend(s.emailSubmissionId).catch(() => undefined); + } const res = await singleton.request([['Email/query', { accountId: singleton.accountId, filter: { subject: tag } }, '0']]); const leftovers = [...new Set([...(res.methodResponses[0][1].ids as string[] ?? []), ...createdEmails])]; if (leftovers.length) await destroyEmails(leftovers).catch(() => undefined); @@ -115,6 +120,62 @@ live('live Stalwart: composer and sending (0.16 and 1.0)', () => { }, 90_000); }); + describe('scheduled sends whose answers get lost', () => { + /** Let the first call matching `pick` reach the server, then lose its answer. */ + const loseAnswerOf = (pick: (method: string, args: Record) => boolean) => { + const real = singleton.request.bind(singleton); + let lost = false; + return vi.spyOn(singleton, 'request').mockImplementation(async (calls, using) => { + const res = await real(calls, using); + const [method, args] = calls[0] as unknown as [string, Record]; + if (!lost && pick(method, args)) { + lost = true; + throw new Error('Request timed out'); + } + return res; + }); + }; + const scheduleOne = async (label: string) => { + const [identity] = await getIdentities(); + const result = await sendEmail( + { from: [{ email: USER }], to: [{ email: USER }], subject: `${label} ${tag}`, textBody: 'live' }, + identity.id, sent, 3600, { draftsMailboxId: drafts }, + ); + createdEmails.push(result.emailId!); + return (await listScheduledEmails()).find((s) => s.emailSubmissionId === result.emailSubmissionId)!; + }; + const pendingOf = async (emailId: string) => (await listScheduledEmails()).filter((s) => s.emailId === emailId); + + it('makes the replacement when the cancel went through and its answer was lost', async () => { + const first = await scheduleOne('lost cancel'); + const spy = loseAnswerOf((method, args) => method === 'EmailSubmission/set' && !!args.update); + let moved: Awaited>; + try { + moved = await rescheduleScheduledSend(first, 7200); + } finally { + spy.mockRestore(); + } + const pending = await pendingOf(first.emailId); + expect(pending.map((s) => s.emailSubmissionId)).toEqual([moved.emailSubmissionId]); + await cancelScheduledToDrafts(pending[0], await getMailboxes(), singleton.accountId); + }, 90_000); + + it('takes the replacement the server made when the answer to its create was lost, and schedules it once', async () => { + const first = await scheduleOne('lost create'); + const spy = loseAnswerOf((method, args) => method === 'EmailSubmission/set' && !!args.create); + let moved: Awaited>; + try { + moved = await rescheduleScheduledSend(first, 7200); + } finally { + spy.mockRestore(); + } + const pending = await pendingOf(first.emailId); + expect(pending.map((s) => s.emailSubmissionId)).toEqual([moved.emailSubmissionId]); + expect(new Date(pending[0].sendAt).getTime()).toBeGreaterThan(new Date(first.sendAt).getTime() + 3_000_000); + await cancelScheduledToDrafts(pending[0], await getMailboxes(), singleton.accountId); + }, 90_000); + }); + describe('Files and mail share blobs', () => { it('saves an attachment to Files and attaches a file from Files, by blob', async () => { const upload = await uploadBytes(new TextEncoder().encode(`hello ${RUN}`), 'text/plain'); diff --git a/src/api/email.ts b/src/api/email.ts index f4c4ecce..48947f51 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -1,6 +1,6 @@ import { jmapClient } from './jmap-client'; import { - assertSetResult, batched, failedIdsOf, JMAPSetError, NoLongerScheduledError, parseHoldLimit, RecipientsRefusedError, + assertSetResult, batched, failedIdsOf, JMAPMethodError, JMAPSetError, NoLongerScheduledError, parseHoldLimit, RecipientsRefusedError, requireMethodResult, ScheduleTooLateError, type RefusedRecipient, } from './jmap-result'; import { keywordPointer, mailboxPointer } from './patch-pointer'; @@ -2146,7 +2146,8 @@ export async function cancelScheduledSend(emailSubmissionId: string, accountIdOv | { type?: string; description?: string } | undefined; if (failure) { - throw new Error(failure.description ?? failure.type ?? 'Failed to cancel scheduled send'); + // A SetError: the server answered and kept the submission as it was. + throw new JMAPSetError(failure.type ?? 'setError', failure.description ?? failure.type ?? 'Failed to cancel scheduled send'); } // The canceled record is dead weight against Stalwart 1.0's submission // limit. It goes in a request of its own, and only once the cancel is @@ -2201,9 +2202,10 @@ export class RescheduleLostError extends Error { * out already (NoLongerScheduledError). A replacement the server refuses, or * takes for no recipient, puts the original back at its old time under a new * id (RescheduleRestoredError); when that fails too the message goes back to - * Drafts (RescheduleLostError). Returns the new submission id and send time, - * and the recipients the server refused for the replacement when it took - * the others. + * Drafts (RescheduleLostError). A cancel or create whose answer was lost is + * checked against the server's records first. Returns the new submission id + * and send time, and the recipients the server refused for the replacement + * when it took the others. */ export async function rescheduleScheduledSend( scheduled: { @@ -2263,6 +2265,9 @@ export async function rescheduleScheduledSend( const sentId = roles.find((m) => m.role === 'sent')?.id; type Created = { id: string; sendAt?: string; delivery: { recipients: RefusedRecipient[]; all: boolean } }; + // Creates that got no answer (timeout, dropped connection): the server + // may have taken them all the same. + const unanswered = new Set(); // `until`: when the submission goes out (ms); undefined, at once. const createSubmission = async (until: number | undefined): Promise => { // HOLDFOR counts from when the server takes the request, so a retry @@ -2295,7 +2300,10 @@ export async function rescheduleScheduledSend( deliveryStatusCall(accountId, 'replacement', '1'), ], SUBMISSION_USING, - ); + ).catch((err: unknown) => { + unanswered.add(err); + throw err; + }); }; let res = await send(); // A full submission quota (Stalwart 1.0): make room and try once more. @@ -2336,8 +2344,17 @@ export async function rescheduleScheduledSend( await cancelScheduledSend(scheduled.emailSubmissionId, accountId); } catch (err) { const status = await submissionUndoStatus(scheduled.emailSubmissionId, accountId); - if (status === 'final' || status === 'canceled') throw new NoLongerScheduledError(status === 'final'); - throw err; + // A cancel the server answered and refused changed nothing, so a + // canceled original was cancelled elsewhere. One whose answer was lost + // (timeout, dropped connection) may have gone through: then the + // original is cancelled by this call and the replacement still has to + // be made, or the message sits in Sent unsent. + const refusedCancel = err instanceof JMAPMethodError; + if (status === 'final' || (status === 'canceled' && refusedCancel)) { + throw new NoLongerScheduledError(status === 'final'); + } + if (status !== 'canceled') throw err; + await destroyEmailSubmissions([scheduled.emailSubmissionId], accountId); } let replacement: Created; @@ -2350,6 +2367,13 @@ export async function rescheduleScheduledSend( const refused = await withdrawIfDead(replacement); if (refused) throw new RecipientsRefusedError(refused); } catch (err) { + // A replacement whose answer was lost may have been taken: putting the + // original back as well would send the message twice. The server's + // record of it stands in for the answer. + if (unanswered.has(err)) { + const taken = await liveSubmissionOf(scheduled.emailId, accountId); + if (taken) return { emailSubmissionId: taken.id, sendAt: taken.sendAt }; + } // Put the original back at its old time: HOLDFOR with the seconds left, // at once when that time has passed meanwhile. const originalTime = original?.sendAt ? Date.parse(original.sendAt) : NaN; @@ -2357,7 +2381,9 @@ export async function rescheduleScheduledSend( try { restored = await createSubmission(Number.isFinite(originalTime) && originalTime > Date.now() ? originalTime : undefined); } catch (restoreErr) { - console.warn('[email] could not put the original scheduled send back:', restoreErr); + const taken = unanswered.has(restoreErr) ? await liveSubmissionOf(scheduled.emailId, accountId) : undefined; + if (taken) restored = { id: taken.id, sendAt: taken.sendAt, delivery: { recipients: [], all: false } }; + else console.warn('[email] could not put the original scheduled send back:', restoreErr); } if (restored && !(await withdrawIfDead(restored).catch(() => null))) { throw new RescheduleRestoredError(err, restored.id, restored.sendAt ?? original?.sendAt); @@ -2396,6 +2422,33 @@ async function readSubmissionUndoStatus(emailSubmissionId: string, accountId: st return list?.find((s) => s.id === emailSubmissionId)?.undoStatus; } +/** + * A submission of `emailId` that is still held or went out, for a create + * whose answer was lost. Undefined when there is none or it can't be read. + */ +async function liveSubmissionOf(emailId: string, accountId: string): Promise<{ id: string; sendAt?: string } | undefined> { + try { + const res = await jmapClient.request( + [ + ['EmailSubmission/query', { accountId, filter: { emailIds: [emailId] } }, '0'], + ['EmailSubmission/get', { + accountId, + '#ids': { resultOf: '0', name: 'EmailSubmission/query', path: '/ids' }, + properties: ['id', 'emailId', 'undoStatus', 'sendAt'], + }, '1'], + ], + SUBMISSION_ONLY_USING, + ); + const list = requireMethodResult(res, '1', 'EmailSubmission/get').list as + Array<{ id?: string; emailId?: string; undoStatus?: string; sendAt?: string }> | undefined; + // "Send now" goes out a second later: a final one is the replacement too. + const live = list?.find((s) => s.id && s.emailId === emailId && (s.undoStatus === 'pending' || s.undoStatus === 'final')); + return live?.id ? { id: live.id, sendAt: live.sendAt } : undefined; + } catch { + return undefined; + } +} + /** A submission's `undoStatus`, or undefined when it can't be read. */ async function submissionUndoStatus(emailSubmissionId: string, accountId: string): Promise { try {