From 7e598c5bc9658956e8fd522cae181ee4d0e78aab Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 19:05:51 +0200 Subject: [PATCH] fix: stop a failed reschedule from dropping or double-sending the message Rescheduling cancelled the original submission and created its replacement in one `EmailSubmission/set`, whose create and update the server applies independently. A refused replacement (a hold over the server's limit, a quota) still cancelled the original, so the message silently never went out, and a refused cancel (the original had just gone out) still let the replacement go, so it was sent twice. Ported the webmail's order: - the replacement is created first; if it is refused, the original stays scheduled - the original is cancelled only once the replacement exists - if the original can't be cancelled, the replacement is withdrawn again, unless a cancel whose answer got lost did go through - "Send now" is a 1-second hold, so the replacement can still be withdrawn --- src/api/__tests__/email-submission.test.ts | 121 ++++++++++++++++++--- src/api/email.ts | 108 ++++++++++++------ 2 files changed, 177 insertions(+), 52 deletions(-) diff --git a/src/api/__tests__/email-submission.test.ts b/src/api/__tests__/email-submission.test.ts index 7511d898..351bd5c2 100644 --- a/src/api/__tests__/email-submission.test.ts +++ b/src/api/__tests__/email-submission.test.ts @@ -31,10 +31,27 @@ const SCHEDULED = { to: [{ email: 'to@example.com' }], }; +const LOOKUP = { + methodResponses: [ + ['EmailSubmission/get', { + list: [{ id: 'sub-1', envelope: { mailFrom: { email: 'me@example.com' }, rcptTo: [{ email: 'to@example.com' }] } }], + }, '0'], + ['Email/get', { list: [{ id: 'e-1', to: [{ email: 'to@example.com' }] }] }, '1'], + ], +}; const REPLACED = [['EmailSubmission/set', { - updated: { 'sub-1': null }, created: { replacement: { id: 'sub-2', sendAt: '2026-09-25T08:00:00Z' } }, }, '0']]; +const set = (body: Record) => ({ methodResponses: [['EmailSubmission/set', body, '0']] }); +const CANCELLED = set({ updated: { 'sub-1': null } }); +/** The EmailSubmission/set calls made, as [create ids, update ids]. */ +const setCalls = () => mockRequest.mock.calls + .map((c) => c[0][0]) + .filter(([method]: [string]) => method === 'EmailSubmission/set') + .map(([, args]: [string, { create?: object; update?: Record }]) => [ + Object.keys(args.create ?? {}), + Object.keys(args.update ?? {}), + ]); describe('rescheduleScheduledSend', () => { it('keeps Cc, Bcc and the envelope sender of the held submission (B19)', async () => { @@ -57,7 +74,8 @@ describe('rescheduleScheduledSend', () => { ['Email/get', { list: [{ id: 'e-1', to: [{ email: 'to@example.com' }], cc: [{ email: 'cc@example.com' }] }] }, '1'], ], }) - .mockResolvedValueOnce({ methodResponses: REPLACED }); + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockResolvedValueOnce(CANCELLED); const result = await rescheduleScheduledSend(SCHEDULED, 7200); @@ -65,7 +83,6 @@ describe('rescheduleScheduledSend', () => { const [lookup] = mockRequest.mock.calls[0][0]; expect(lookup).toEqual(['EmailSubmission/get', { accountId: 'acc-1', ids: ['sub-1'], properties: ['envelope'] }, '0']); const args = mockRequest.mock.calls[1][0][0][1]; - expect(args.update).toEqual({ 'sub-1': { undoStatus: 'canceled' } }); expect(args.create.replacement.envelope).toEqual({ mailFrom: { email: 'me@example.com', parameters: { HOLDFOR: '7200' } }, rcptTo: [{ email: 'to@example.com' }, { email: 'cc@example.com' }, { email: 'bcc@example.com' }], @@ -87,7 +104,8 @@ describe('rescheduleScheduledSend', () => { }, '1'], ], }) - .mockResolvedValueOnce({ methodResponses: REPLACED }); + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockResolvedValueOnce(CANCELLED); await rescheduleScheduledSend(SCHEDULED, 60); @@ -97,13 +115,84 @@ describe('rescheduleScheduledSend', () => { }); }); - it('sends now without a lookup and lets the server derive the envelope', async () => { - mockRequest.mockResolvedValueOnce({ methodResponses: REPLACED }); + it('creates the replacement before cancelling the original', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockResolvedValueOnce(CANCELLED); + + await rescheduleScheduledSend(SCHEDULED, 7200); + + expect(setCalls()).toEqual([[['replacement'], []], [[], ['sub-1']]]); + expect(mockRequest.mock.calls[2][0][0][1].update).toEqual({ 'sub-1': { undoStatus: 'canceled' } }); + }); + + it('sends now as a 1-second hold, so the replacement can still be withdrawn', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockResolvedValueOnce(CANCELLED); await rescheduleScheduledSend(SCHEDULED, 0); - expect(mockRequest).toHaveBeenCalledTimes(1); - expect(mockRequest.mock.calls[0][0][0][1].create.replacement).toEqual({ emailId: 'e-1', identityId: 'id-1' }); + const replacement = mockRequest.mock.calls[1][0][0][1].create.replacement; + expect(replacement.envelope).toEqual({ + mailFrom: { email: 'me@example.com', parameters: { HOLDFOR: '1' } }, + rcptTo: [{ email: 'to@example.com' }], + }); + expect(setCalls()).toEqual([[['replacement'], []], [[], ['sub-1']]]); + }); + + it('leaves the original scheduled when the replacement is refused', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(set({ notCreated: { replacement: { type: 'forbiddenToSend', description: 'Quota exceeded' } } })); + + await expect(rescheduleScheduledSend(SCHEDULED, 7200)).rejects.toThrow('Quota exceeded'); + // No cancel went out. + expect(setCalls()).toEqual([[['replacement'], []]]); + }); + + it('withdraws the replacement when the original has already gone out', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockResolvedValueOnce(set({ notUpdated: { 'sub-1': { type: 'cannotUnsend', description: 'Already sent' } } })) + .mockResolvedValueOnce({ methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus: 'final' }] }, '0']] }) + .mockResolvedValueOnce(set({ updated: { 'sub-2': null } })); + + await expect(rescheduleScheduledSend(SCHEDULED, 0)).rejects.toThrow('Failed to cancel the previous schedule: Already sent'); + expect(setCalls()).toEqual([[['replacement'], []], [[], ['sub-1']], [[], ['sub-2']]]); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('still reports the failure when the replacement cannot be withdrawn either', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockResolvedValueOnce(set({ notUpdated: { 'sub-1': { type: 'cannotUnsend' } } })) + .mockResolvedValueOnce({ methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus: 'final' }] }, '0']] }) + .mockResolvedValueOnce(set({ notUpdated: { 'sub-2': { type: 'cannotUnsend' } } })); + + await expect(rescheduleScheduledSend(SCHEDULED, 0)).rejects.toThrow('Failed to cancel the previous schedule'); + expect(warn).toHaveBeenCalledTimes(1); + warn.mockRestore(); + }); + + it('keeps the replacement when a cancel whose answer got lost did go through', async () => { + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce({ methodResponses: REPLACED }) + .mockRejectedValueOnce(new Error('Network request failed')) + .mockResolvedValueOnce({ methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus: 'canceled' }] }, '0']] }); + + const result = await rescheduleScheduledSend(SCHEDULED, 7200); + + expect(result.emailSubmissionId).toBe('sub-2'); + expect(setCalls()).toEqual([[['replacement'], []], [[], ['sub-1']]]); }); }); @@ -216,13 +305,15 @@ describe('hold-limit rejections', () => { ], }) .mockResolvedValueOnce({ - methodResponses: [['EmailSubmission/set', { updated: { 'sub-1': null }, notCreated: { replacement: HOLD_REJECTED } }, '0']], + methodResponses: [['EmailSubmission/set', { notCreated: { replacement: HOLD_REJECTED } }, '0']], }); const err = await rescheduleScheduledSend(SCHEDULED, 5 * 24 * 3600).catch((e) => e); expect(err).toBeInstanceOf(ScheduleTooLateError); expect(jmapClient.learnHoldLimit).toHaveBeenCalledWith(172_800); + // The original stays scheduled. + expect(setCalls()).toEqual([[['replacement'], []]]); }); }); @@ -301,13 +392,11 @@ describe('scheduled sends in shared accounts (webmail #874)', () => { await cancelScheduledSend('s-grp', 'grp-1'); expect(mockRequest.mock.calls[0][0][0][1].accountId).toBe('grp-1'); - mockRequest.mockResolvedValueOnce({ - methodResponses: [['EmailSubmission/set', { - updated: { 's-grp': null }, - created: { replacement: { id: 's-new', sendAt: FUTURE } }, - }, '0']], - }); + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(set({ created: { replacement: { id: 's-new', sendAt: FUTURE } } })) + .mockResolvedValueOnce(set({ updated: { 's-grp': null } })); await rescheduleScheduledSend({ ...SCHEDULED, emailSubmissionId: 's-grp', accountId: 'grp-1' }, 0); - expect(mockRequest.mock.calls[1][0][0][1].accountId).toBe('grp-1'); + expect(mockRequest.mock.calls.slice(1).map((c) => c[0][0][1].accountId)).toEqual(['grp-1', 'grp-1', 'grp-1']); }); }); diff --git a/src/api/email.ts b/src/api/email.ts index 158a56b8..8db93274 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -1531,10 +1531,16 @@ export async function cancelScheduledSend(emailSubmissionId: string, accountIdOv } /** - * Change when a scheduled message goes out (or send it now with `holdForSeconds` - * = 0): cancel the pending submission and create a replacement for the same - * Email with the held envelope and a fresh HOLDFOR, in one request. Returns the new - * submission id and resolved send time. + * Change when a scheduled message goes out, or send it now with + * `holdForSeconds` = 0. A replacement submission for the same Email, with the + * held envelope and a fresh HOLDFOR, is created first, and the original is + * cancelled only once the replacement exists (webmail parity). Both used to + * go in one /set, whose create and update the server applies independently: + * a refused replacement still cancelled the original, so the message never + * went out, and a refused cancel (the original had just gone out) still let + * the replacement go, so it was sent twice. Now a refused replacement leaves + * the original alone, and a replacement whose original can't be cancelled is + * withdrawn again. Returns the new submission id and send time. */ export async function rescheduleScheduledSend( scheduled: { @@ -1550,49 +1556,79 @@ export async function rescheduleScheduledSend( recipients?: EmailAddress[], ): Promise<{ emailSubmissionId?: string; sendAt?: string }> { const accountId = scheduled.accountId ?? jmapClient.accountId; - const create: Record = { emailId: scheduled.emailId, identityId: scheduled.identityId }; - if (holdForSeconds > 0) { - // Reuse the held submission's envelope: it names every recipient, - // Cc and Bcc included, and the envelope sender a catch-all From went - // out through. Rebuilding it from the Email's To dropped Cc and Bcc. - // The Email's To/Cc/Bcc are the fallback when the server has none. - const lookup = await jmapClient.request( - [ - ['EmailSubmission/get', { accountId, ids: [scheduled.emailSubmissionId], properties: ['envelope'] }, '0'], - ['Email/get', { accountId, ids: [scheduled.emailId], properties: ['to', 'cc', 'bcc'] }, '1'], - ], - [CAPABILITIES.CORE, CAPABILITIES.MAIL, CAPABILITIES.SUBMISSION], - ); - const envelope = (requireMethodResult(lookup, '0', 'EmailSubmission/get').list as Array<{ - envelope?: { mailFrom?: { email?: string }; rcptTo?: Array<{ email?: string }> } | null; - }> | undefined)?.[0]?.envelope; - const held = requireMethodResult(lookup, '1', 'Email/get').list as Email[] | undefined; - const headerRecipients = [...(held?.[0]?.to ?? []), ...(held?.[0]?.cc ?? []), ...(held?.[0]?.bcc ?? [])]; - const source = recipients ?? (envelope?.rcptTo?.length ? envelope.rcptTo : headerRecipients); - const rcpt = source.map((r) => ({ email: (r.email ?? '').trim() })).filter((r) => r.email); - create.envelope = { + // "Send now" is a 1-second hold, so the replacement can still be withdrawn. + const holdFor = Math.max(1, Math.ceil(holdForSeconds)); + // Reuse the held submission's envelope: it names every recipient, + // Cc and Bcc included, and the envelope sender a catch-all From went + // out through. Rebuilding it from the Email's To dropped Cc and Bcc. + // The Email's To/Cc/Bcc are the fallback when the server has none. + const lookup = await jmapClient.request( + [ + ['EmailSubmission/get', { accountId, ids: [scheduled.emailSubmissionId], properties: ['envelope'] }, '0'], + ['Email/get', { accountId, ids: [scheduled.emailId], properties: ['to', 'cc', 'bcc'] }, '1'], + ], + [CAPABILITIES.CORE, CAPABILITIES.MAIL, CAPABILITIES.SUBMISSION], + ); + const envelope = (requireMethodResult(lookup, '0', 'EmailSubmission/get').list as Array<{ + envelope?: { mailFrom?: { email?: string }; rcptTo?: Array<{ email?: string }> } | null; + }> | undefined)?.[0]?.envelope; + const held = requireMethodResult(lookup, '1', 'Email/get').list as Email[] | undefined; + const headerRecipients = [...(held?.[0]?.to ?? []), ...(held?.[0]?.cc ?? []), ...(held?.[0]?.bcc ?? [])]; + const source = recipients ?? (envelope?.rcptTo?.length ? envelope.rcptTo : headerRecipients); + const rcpt = source.map((r) => ({ email: (r.email ?? '').trim() })).filter((r) => r.email); + const create = { + emailId: scheduled.emailId, + identityId: scheduled.identityId, + envelope: { mailFrom: { email: envelope?.mailFrom?.email || scheduled.from?.[0]?.email, - parameters: { HOLDFOR: String(Math.ceil(holdForSeconds)) }, + parameters: { HOLDFOR: String(holdFor) }, }, rcptTo: rcpt, - }; - } + }, + }; + const res = await jmapClient.request( - [['EmailSubmission/set', { - accountId, - update: { [scheduled.emailSubmissionId]: { undoStatus: 'canceled' } }, - create: { replacement: create }, - }, '0']], + [['EmailSubmission/set', { accountId, create: { replacement: create } }, '0']], [CAPABILITIES.CORE, CAPABILITIES.SUBMISSION], ); const body = requireMethodResult(res, '0', 'EmailSubmission/set'); - const notUpdated = body.notUpdated?.[scheduled.emailSubmissionId] as { description?: string; type?: string } | undefined; - if (notUpdated) throw new Error(notUpdated.description ?? notUpdated.type ?? 'Failed to cancel the previous schedule'); const notCreated = body.notCreated?.replacement as { description?: string; type?: string } | undefined; if (notCreated) throw submissionError(notCreated, 'Failed to reschedule'); const created = body.created?.replacement as { id?: string; sendAt?: string } | undefined; - return { emailSubmissionId: created?.id, sendAt: created?.sendAt }; + if (!created?.id) throw new Error('Failed to reschedule: the server returned no submission'); + + try { + await cancelScheduledSend(scheduled.emailSubmissionId, accountId); + } catch (err) { + // A cancel whose answer got lost may still have gone through. Otherwise + // the original is still due or has just gone out, and the replacement + // must not go as well. + if ((await submissionUndoStatus(scheduled.emailSubmissionId, accountId)) !== 'canceled') { + try { + await cancelScheduledSend(created.id, accountId); + } catch (withdrawErr) { + console.warn('[email] could not withdraw the replacement scheduled send:', withdrawErr); + } + const reason = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to cancel the previous schedule: ${reason}`); + } + } + return { emailSubmissionId: created.id, sendAt: created.sendAt }; +} + +/** A submission's `undoStatus`, or undefined when it can't be read. */ +async function submissionUndoStatus(emailSubmissionId: string, accountId: string): Promise { + try { + const res = await jmapClient.request( + [['EmailSubmission/get', { accountId, ids: [emailSubmissionId], properties: ['undoStatus'] }, '0']], + [CAPABILITIES.CORE, CAPABILITIES.SUBMISSION], + ); + const list = requireMethodResult(res, '0', 'EmailSubmission/get').list as Array<{ undoStatus?: string }> | undefined; + return list?.[0]?.undoStatus; + } catch { + return undefined; + } } /**