diff --git a/locales/rn/en.json b/locales/rn/en.json index 194a134d..370bc0b5 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -697,6 +697,8 @@ "keep_scheduled": "Keep scheduled", "no_longer_scheduled": "This message is no longer scheduled.", "reschedule_failed": "Reschedule failed", + "reschedule_lost": "The message is no longer scheduled. Check Drafts and Sent before sending it again.", + "reschedule_lost_drafts": "The message is no longer scheduled. It is back in Drafts, where you can send it again.", "send_now_failed": "Could not send now" }, "settings": { diff --git a/src/api/__tests__/email-submission-cleanup.test.ts b/src/api/__tests__/email-submission-cleanup.test.ts index b28f8df9..30669b93 100644 --- a/src/api/__tests__/email-submission-cleanup.test.ts +++ b/src/api/__tests__/email-submission-cleanup.test.ts @@ -40,7 +40,7 @@ vi.mock('../blob', () => ({ import { jmapClient } from '../jmap-client'; import { - cancelScheduledSend, listScheduledEmails, rescheduleScheduledSend, sendEmail, sendReadReceipt, + cancelScheduledSend, listScheduledEmails, RescheduleLostError, rescheduleScheduledSend, sendEmail, sendReadReceipt, } from '../email'; import { NoLongerScheduledError, SubmissionQuotaError } from '../jmap-result'; import { prunableSubmissionIds, pruneEmailSubmissions, resetSubmissionPrune } from '../submissions'; @@ -456,7 +456,9 @@ describe('rescheduling and "Send now"', () => { expect(argsOf(server, 'EmailSubmission/set').map((a) => a.destroy).filter(Boolean)).toEqual([[original]]); }); - it('Stalwart 1.0: a full quota makes room without touching the pending original', async () => { + // The original is cancelled and its record removed first (#1308), so the + // replacement takes the room the original had. + it('Stalwart 1.0: a full account has room for the replacement once the original is cancelled', async () => { const server = setup('1.0', { limit: 3 }); const now = Date.now(); const [original] = server.seed('acc-1', [{ sendAt: now + HOUR, status: 'pending', emailId: 'mail-1' }]); @@ -464,24 +466,42 @@ describe('rescheduling and "Send now"', () => { const result = await rescheduleScheduledSend(scheduled(original), 2 * 3600); - expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(2); - expect(server.submissions()).toEqual([{ id: result.emailSubmissionId, sendAt: expect.any(Number), undoStatus: 'pending' }]); + expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(1); + expect(server.submissions().filter((s) => s.undoStatus === 'pending')).toEqual([ + { id: result.emailSubmissionId, sendAt: expect.any(Number), undoStatus: 'pending' }, + ]); + expect(server.submissions().some((s) => s.id === original)).toBe(false); }); - it('Stalwart 1.0: still full after making room, the original stays scheduled', async () => { - const server = setup('1.0', { limit: 1 }); - const [original] = server.seed('acc-1', [{ sendAt: Date.now() + HOUR, status: 'pending', emailId: 'mail-1' }]); + it('Stalwart 1.0: an original that went out before the cancel is not sent a second time', async () => { + const server = setup('1.0'); + const [original] = server.seed('acc-1', [{ sendAt: Date.now() + 30_000, status: 'pending', emailId: 'mail-1' }]); + // Its HOLDFOR runs out while the lookup is under way. + server.afterEachRequest(() => server.deliver('acc-1', original)); const err = await rescheduleScheduledSend(scheduled(original), 0).catch((e: unknown) => e); - expect(err).toBeInstanceOf(SubmissionQuotaError); - expect(argsOf(server, 'EmailSubmission/set').some((a) => a.update)).toBe(false); - expect(server.submissions()).toEqual([{ id: original, sendAt: expect.any(Number), undoStatus: 'pending' }]); + expect(err).toBeInstanceOf(NoLongerScheduledError); + expect(err).toMatchObject({ sent: true }); + expect(argsOf(server, 'EmailSubmission/set').some((a) => a.create)).toBe(false); + expect(server.submissions()).toEqual([{ id: original, sendAt: expect.any(Number), undoStatus: 'final' }]); }); - /** A full account: the original, due soon, and two delivered records the prune can remove. */ - function fullWithOriginal(dueIn: number) { - const server = setup('1.0', { limit: 3 }); + it('Stalwart 1.0: an original cancelled on another device meanwhile is left as it is', async () => { + const server = setup('1.0'); + const [original] = server.seed('acc-1', [{ sendAt: Date.now() + HOUR, status: 'pending', emailId: 'mail-1' }]); + server.afterEachRequest(() => server.cancel('acc-1', original)); + + const err = await rescheduleScheduledSend(scheduled(original), 2 * 3600).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(NoLongerScheduledError); + expect(err).toMatchObject({ sent: false }); + expect(argsOf(server, 'EmailSubmission/set').some((a) => a.create)).toBe(false); + }); + + /** An account at its limit once the original is gone: two delivered records the prune can remove. */ + function fullAfterCancel(dueIn: number) { + const server = setup('1.0', { limit: 2 }); const now = Date.now(); const [original] = server.seed('acc-1', [{ sendAt: now + dueIn, status: 'pending', emailId: 'mail-1' }]); server.seed('acc-1', [{ sendAt: now - HOUR, status: 'final' }, { sendAt: now - 2 * HOUR, status: 'final' }]); @@ -489,59 +509,10 @@ describe('rescheduling and "Send now"', () => { } const isQuery = (calls: Array<[string, unknown, string]>) => calls[0][0] === 'EmailSubmission/query'; - it('Stalwart 1.0: an original that went out while room was made is not sent a second time', async () => { - const { server, original } = fullWithOriginal(30_000); - // Its HOLDFOR runs out while the prune looks for records to remove. - server.afterEachRequest((calls) => { - if (isQuery(calls)) server.deliver('acc-1', original); - }); - - const err = await rescheduleScheduledSend(scheduled(original), 0).catch((e: unknown) => e); - - expect(err).toBeInstanceOf(NoLongerScheduledError); - expect(err).toMatchObject({ sent: true }); - // The original was read again after the prune, and only the refused create was made. - expect(server.requests.at(-1)?.calls).toEqual([ - ['EmailSubmission/get', { accountId: 'acc-1', ids: [original], properties: ['id', 'undoStatus'] }, '0'], - ]); - expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(1); - expect(argsOf(server, 'EmailSubmission/set').some((a) => a.update)).toBe(false); - expect(server.submissions()).toEqual([{ id: original, sendAt: expect.any(Number), undoStatus: 'final' }]); - }); - - it('Stalwart 1.0: an original cancelled on another device meanwhile is left as it is', async () => { - const { server, original } = fullWithOriginal(HOUR); - server.afterEachRequest((calls) => { - if (isQuery(calls)) server.cancel('acc-1', original); - }); - - const err = await rescheduleScheduledSend(scheduled(original), 2 * 3600).catch((e: unknown) => e); - - expect(err).toBeInstanceOf(NoLongerScheduledError); - expect(err).toMatchObject({ sent: false }); - expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(1); - expect(server.submissions()).toEqual([{ id: original, sendAt: expect.any(Number), undoStatus: 'canceled' }]); - }); - - it('Stalwart 1.0: when the original cannot be read again, nothing is resubmitted', async () => { - const { server, original } = fullWithOriginal(HOUR); - server.beforeEachRequest((calls) => { - const [name, args] = calls[0]; - if (name === 'EmailSubmission/get' && (args as { properties?: string[] }).properties?.join() === 'id,undoStatus') { - throw new Error('Network request failed'); - } - }); - - await expect(rescheduleScheduledSend(scheduled(original), 2 * 3600)).rejects.toThrow('Network request failed'); - - expect(argsOf(server, 'EmailSubmission/set').filter((a) => a.create)).toHaveLength(1); - expect(server.submissions()).toEqual([{ id: original, sendAt: expect.any(Number), undoStatus: 'pending' }]); - }); - it('Stalwart 1.0: the retry takes the time spent making room off the hold', async () => { let clock = Date.parse('2026-09-25T12:00:00Z'); vi.spyOn(Date, 'now').mockImplementation(() => clock); - const { server, original } = fullWithOriginal(HOUR); + const { server, original } = fullAfterCancel(HOUR); const target = clock + 2 * HOUR; // The prune takes five minutes. server.afterEachRequest((calls) => { @@ -561,7 +532,7 @@ describe('rescheduling and "Send now"', () => { it('Stalwart 1.0: "Send now" that waited for room still goes out at once', async () => { let clock = Date.parse('2026-09-25T12:00:00Z'); vi.spyOn(Date, 'now').mockImplementation(() => clock); - const { server, original } = fullWithOriginal(HOUR); + const { server, original } = fullAfterCancel(HOUR); server.afterEachRequest((calls) => { if (isQuery(calls)) clock += 5 * MINUTE; }); @@ -573,6 +544,22 @@ describe('rescheduling and "Send now"', () => { .map((a) => a.create.replacement.envelope.mailFrom.parameters.HOLDFOR); expect(holds).toEqual(['1', '1']); }); + + it('Stalwart 1.0: still full after making room, the original goes back to its old time', async () => { + const server = setup('1.0', { limit: 2 }); + const now = Date.now(); + const [original] = server.seed('acc-1', [{ sendAt: now + HOUR, status: 'pending', emailId: 'mail-1' }]); + // Two other held messages: nothing the prune may remove. + server.seed('acc-1', [{ sendAt: now + 2 * HOUR, status: 'pending' }, { sendAt: now + 3 * HOUR, status: 'pending' }]); + + const err = await rescheduleScheduledSend(scheduled(original), 0).catch((e: unknown) => e); + + // Neither the replacement nor the original found room: the message is + // back in Drafts rather than in Sent looking sent. + expect(err).toBeInstanceOf(RescheduleLostError); + expect((err as RescheduleLostError).reason).toBeInstanceOf(SubmissionQuotaError); + expect(server.emails.get('mail-1')).toMatchObject({ mailboxIds: { drafts: true }, keywords: { $draft: true } }); + }); }); describe('read receipts', () => { diff --git a/src/api/__tests__/email-submission-refusals.test.ts b/src/api/__tests__/email-submission-refusals.test.ts index 76ce798b..5ebc227d 100644 --- a/src/api/__tests__/email-submission-refusals.test.ts +++ b/src/api/__tests__/email-submission-refusals.test.ts @@ -36,7 +36,9 @@ vi.mock('../jmap-client', () => ({ })); import { jmapClient } from '../jmap-client'; -import { listScheduledEmails, rescheduleScheduledSend, sendEmail } from '../email'; +import { + listScheduledEmails, RescheduleLostError, RescheduleRestoredError, rescheduleScheduledSend, sendEmail, +} from '../email'; import { RecipientsRefusedError, jmapErrorType } from '../jmap-result'; import { pruneEmailSubmissions, resetSubmissionPrune } from '../submissions'; import { @@ -263,20 +265,24 @@ describe('"Send now" and "Reschedule" when the server refuses the recipients aga const cancelled = (server: FakeSubmissionServer) => argsOf(server, 'EmailSubmission/set') .filter((a) => a.update) .flatMap((a) => Object.keys(a.update as object)); - /** The replacements the server created, in order. */ - const replacements = (server: FakeSubmissionServer) => server.requests - .flatMap(({ responses }) => responses) - .filter(([name, body]) => name === 'EmailSubmission/set' && body.created?.replacement) - .map(([, body]) => body.created.replacement.id as string); + beforeEach(() => { + // "Send now" holds the replacement for a second: the clock stands still. + vi.spyOn(Date, 'now').mockReturnValue(Date.parse('2026-09-26T10:00:00Z')); + }); + + // The original is cancelled before the replacement is made (#1308). A + // replacement that takes nobody puts the original back; when the throttle + // refuses that one too, the message goes back to Drafts and says so. it.each([ ['1.0', 0], ['1.0', 7200], ['0.16.23', 0], ['0.16.23', 7200], ] as Array<[StalwartVersion, number]>)( - 'Stalwart %s: keeps the original when every recipient of the replacement is refused (hold %is)', + 'Stalwart %s: moves the message back to Drafts when every recipient is refused again (hold %is)', async (version, holdFor) => { + vi.spyOn(console, 'warn').mockImplementation(() => undefined); const server = setup(version); const scheduled = scheduledTo(server, 'bob@example.org'); // A throttle used up since the message was scheduled. @@ -284,13 +290,32 @@ describe('"Send now" and "Reschedule" when the server refuses the recipients aga const err = await rescheduleScheduledSend(scheduled, holdFor).catch((e: unknown) => e); - expect(err).toBeInstanceOf(RecipientsRefusedError); - expect((err as RecipientsRefusedError).recipients).toEqual([{ email: 'bob@example.org', smtpReply: RATE_LIMITED }]); - // Still goes at its old time; no cancel went out, and the dead - // replacement's record is gone. - expect(argsOf(server, 'EmailSubmission/set').some((a) => a.update)).toBe(false); + expect(err).toBeInstanceOf(RescheduleLostError); + const reason = (err as RescheduleLostError).reason; + expect(reason).toBeInstanceOf(RecipientsRefusedError); + expect((reason as RecipientsRefusedError).recipients).toEqual([{ email: 'bob@example.org', smtpReply: RATE_LIMITED }]); + expect(server.emails.get('mail-1')).toMatchObject({ mailboxIds: { drafts: true }, keywords: { $draft: true } }); + // The dead records are gone, and nothing is pending. + expect(server.submissions()).toEqual([]); + }, + ); + + it.each(['1.0', '0.16.23'] as StalwartVersion[])( + 'Stalwart %s: puts the original back at its old time when the replacement took nobody but the original is taken again', + async (version) => { + const server = setup(version); + const scheduled = scheduledTo(server, 'bob@example.org'); + // Refused once, for the replacement; the throttle has room again for the original. + let refusals = 1; + server.refuseRecipients(() => (refusals-- > 0 ? RATE_LIMITED : undefined)); + + const err = await rescheduleScheduledSend(scheduled, 7200).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(RescheduleRestoredError); + const restored = err as RescheduleRestoredError; + expect(restored.reason).toBeInstanceOf(RecipientsRefusedError); expect(server.submissions()).toEqual([ - { id: scheduled.emailSubmissionId, sendAt: expect.any(Number), undoStatus: 'pending' }, + { id: restored.restoredSubmissionId, sendAt: Date.now() + HOUR, undoStatus: 'pending' }, ]); }, ); @@ -305,7 +330,9 @@ describe('"Send now" and "Reschedule" when the server refuses the recipients aga expect(create?.calls[1]).toEqual( ['EmailSubmission/get', { accountId: 'acc-1', ids: ['#replacement'], properties: ['id', 'deliveryStatus'] }, '1'], ); - expect(create?.using).toEqual(['urn:ietf:params:jmap:core', 'urn:ietf:params:jmap:submission']); + expect(create?.using).toEqual([ + 'urn:ietf:params:jmap:core', 'urn:ietf:params:jmap:mail', 'urn:ietf:params:jmap:submission', + ]); }); it.each(['1.0', '0.16.23'] as StalwartVersion[])( @@ -324,141 +351,51 @@ describe('"Send now" and "Reschedule" when the server refuses the recipients aga }, ); - it('leaves the refused replacement to the prune on a server without urn:stalwart:jmap', async () => { + it.each([ + ['1.0', 0], + ['1.0', 7200], + ['0.16.23', 0], + ['0.16.6', 7200], + ] as Array<[StalwartVersion, number]>)( + 'Stalwart %s: goes ahead and names the recipient the replacement was refused while it took the others (hold %is)', + async (version, holdFor) => { + const server = setup(version); + const scheduled = scheduledTo(server, 'alice@example.org', 'bob@example.org'); + server.refuseRecipients(throttledFor('bob@example.org')); + + const result = await rescheduleScheduledSend(scheduled, holdFor); + + // The original is gone before the replacement is made, so the caller + // warns that bob does not get it. + expect(result.refusedRecipients).toEqual([{ email: 'bob@example.org', smtpReply: RATE_LIMITED }]); + expect(cancelled(server)).toEqual([scheduled.emailSubmissionId]); + expect(server.submissions()).toEqual([ + { id: result.emailSubmissionId, sendAt: expect.any(Number), undoStatus: 'pending' }, + ]); + }, + ); + + it('leaves the dead records to the prune on a server without urn:stalwart:jmap', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => undefined); fake.stalwart = false; const server = setup('1.0'); const scheduled = scheduledTo(server, 'bob@example.org'); server.refuseRecipients(() => RATE_LIMITED); - await expect(rescheduleScheduledSend(scheduled, 0)).rejects.toBeInstanceOf(RecipientsRefusedError); + await expect(rescheduleScheduledSend(scheduled, 0)).rejects.toBeInstanceOf(RescheduleLostError); - expect(server.submissions().map((s) => s.undoStatus)).toEqual(['pending', 'final']); + // The replacement's and the put-back original's, both final with nobody taken. + expect(server.submissions().map((s) => s.undoStatus)).toEqual(['final', 'final']); }); - // The replacement takes the others: cancelling the original would lose - // the message for the refused one, whom the original still reaches. - describe('when the replacement is refused a recipient the original was taken for', () => { - beforeEach(() => { - // "Send now" holds the replacement for a second, so the clock stands - // still for the withdraw to find it held. - vi.spyOn(Date, 'now').mockReturnValue(Date.parse('2026-09-26T10:00:00Z')); - }); + it('files the replacement in Sent, for servers that pull a cancelled message back to Drafts', async () => { + const server = setup('1.0'); + const scheduled = scheduledTo(server, 'bob@example.org'); + server.emails.set('mail-1', { ...server.emails.get('mail-1')!, mailboxIds: { drafts: true }, keywords: { $draft: true } }); - it.each([ - ['1.0', 0], - ['1.0', 7200], - ['0.16.23', 0], - ['0.16.23', 7200], - ['0.16.6', 0], - ] as Array<[StalwartVersion, number]>)( - 'Stalwart %s: keeps the original at its old time and withdraws the replacement (hold %is)', - async (version, holdFor) => { - const server = setup(version); - const scheduled = scheduledTo(server, 'alice@example.org', 'bob@example.org'); - server.refuseRecipients(throttledFor('bob@example.org')); + await rescheduleScheduledSend(scheduled, 7200); - const err = await rescheduleScheduledSend(scheduled, holdFor).catch((e: unknown) => e); - - expect(err).toBeInstanceOf(RecipientsRefusedError); - expect((err as RecipientsRefusedError).recipients).toEqual([{ email: 'bob@example.org', smtpReply: RATE_LIMITED }]); - // Bob still gets it at the old time, and alice only once: the - // replacement she was taken for is cancelled, not just its record. - expect(cancelled(server)).toEqual(replacements(server)); - expect(server.submissions()).toEqual([ - { id: scheduled.emailSubmissionId, sendAt: Date.now() + HOUR, undoStatus: 'pending' }, - ]); - }, - ); - - it.each(['1.0', '0.16.23'] as StalwartVersion[])( - 'Stalwart %s: names only whom the replacement would have lost when it took nobody', - async (version) => { - const server = setup(version, { refuseRecipient: relayOff }); - const scheduled = scheduledTo(server, 'alice@example.org', 'nobody@example.net'); - server.refuseRecipients((email) => relayOff(email) ?? throttledFor('alice@example.org')(email)); - - const err = await rescheduleScheduledSend(scheduled, 7200).catch((e: unknown) => e); - - // nobody@example.net never got it: the relay rule refused it back then. - expect((err as RecipientsRefusedError).recipients).toEqual([{ email: 'alice@example.org', smtpReply: RATE_LIMITED }]); - // Nothing was queued for the replacement, so only its record goes. - expect(cancelled(server)).toEqual([]); - expect(server.submissions()).toEqual([ - { id: scheduled.emailSubmissionId, sendAt: Date.now() + HOUR, undoStatus: 'pending' }, - ]); - }, - ); - - it('Stalwart 1.0 shapes: counts every recipient as taken when the server keeps no delivery status for the original', async () => { - const server = setup('1.0', { refuseRecipient: relayOff }); - const scheduled = scheduledTo(server, 'alice@example.org', 'nobody@example.net'); - vi.mocked(jmapClient.request).mockImplementationOnce(async (calls, using) => { - const res = await server.request(calls, using); - return { - methodResponses: res.methodResponses.map(([name, body, callId]) => (name === 'EmailSubmission/get' - ? [name, { ...body, list: body.list.map((s: object) => ({ ...s, deliveryStatus: null })) }, callId] - : [name, body, callId])) as Array<[string, Record, string]>, - }; - }); - - const err = await rescheduleScheduledSend(scheduled, 7200).catch((e: unknown) => e); - - // Nothing says the relay rule refused nobody@example.net back then. - expect((err as RecipientsRefusedError).recipients).toEqual([{ email: 'nobody@example.net', smtpReply: RELAY_DENIED }]); - expect(cancelled(server)).toEqual(replacements(server)); - expect(server.submissions().map((s) => [s.id, s.undoStatus])).toEqual([[scheduled.emailSubmissionId, 'pending']]); - }); - - it.each(['1.0', '0.16.23'] as StalwartVersion[])( - 'Stalwart %s: goes ahead and names the refused recipient when "Send now" went out before it could be withdrawn', - async (version) => { - vi.spyOn(console, 'warn').mockImplementation(() => undefined); - const server = setup(version); - const scheduled = scheduledTo(server, 'alice@example.org', 'bob@example.org'); - server.refuseRecipients(throttledFor('bob@example.org')); - // Its second of hold is over before the withdraw arrives. - server.afterEachRequest(() => { - for (const id of replacements(server)) server.deliver('acc-1', id); - }); - - const result = await rescheduleScheduledSend(scheduled, 0); - - // Alice has it from the replacement: keeping the original as well - // would send her a second copy. - expect(result.refusedRecipients).toEqual([{ email: 'bob@example.org', smtpReply: RATE_LIMITED }]); - expect(cancelled(server)).toEqual([result.emailSubmissionId, scheduled.emailSubmissionId]); - expect(server.submissions()).toEqual([{ id: result.emailSubmissionId, sendAt: expect.any(Number), undoStatus: 'final' }]); - }, - ); - - it.each(['1.0', '0.16.23'] as StalwartVersion[])( - 'Stalwart %s: keeps the original when the withdraw went through and only its answer was lost', - async (version) => { - const server = setup(version); - const scheduled = scheduledTo(server, 'alice@example.org', 'bob@example.org'); - server.refuseRecipients(throttledFor('bob@example.org')); - let answered = false; - server.afterEachRequest((calls) => { - const [name, args] = calls[0]; - if (name === 'EmailSubmission/set' && (args as { update?: object }).update && !answered) { - answered = true; - throw new Error('Network request failed'); - } - }); - - const err = await rescheduleScheduledSend(scheduled, 7200).catch((e: unknown) => e); - - // Going ahead would cancel the original too: nobody would get it. - expect(err).toBeInstanceOf(RecipientsRefusedError); - const [replacement] = replacements(server); - expect(cancelled(server)).toEqual([replacement]); - // The canceled record is left to the prune. - expect(server.submissions().map((s) => [s.id, s.undoStatus])).toEqual([ - [scheduled.emailSubmissionId, 'pending'], - [replacement, 'canceled'], - ]); - }, - ); + expect(server.emails.get('mail-1')).toMatchObject({ mailboxIds: { sent: true }, keywords: {} }); }); }); diff --git a/src/api/__tests__/email-submission.test.ts b/src/api/__tests__/email-submission.test.ts index 7306ba4b..f0f55ded 100644 --- a/src/api/__tests__/email-submission.test.ts +++ b/src/api/__tests__/email-submission.test.ts @@ -18,9 +18,10 @@ vi.mock('../jmap-client', () => ({ import { jmapClient } from '../jmap-client'; import { - cancelScheduledSend, listScheduledEmails, rescheduleScheduledSend, sendEmail, SendUnconfirmedError, + cancelScheduledSend, listScheduledEmails, RescheduleLostError, RescheduleRestoredError, rescheduleScheduledSend, + sendEmail, SendUnconfirmedError, } from '../email'; -import { ScheduleTooLateError } from '../jmap-result'; +import { NoLongerScheduledError, ScheduleTooLateError } from '../jmap-result'; const mockRequest = jmapClient.request as ReturnType; @@ -36,12 +37,18 @@ const SCHEDULED = { to: [{ email: 'to@example.com' }], }; +const ORIGINAL_AT = new Date(Date.now() + 3600_000).toISOString(); const LOOKUP = { methodResponses: [ ['EmailSubmission/get', { - list: [{ id: 'sub-1', envelope: { mailFrom: { email: 'me@example.com' }, rcptTo: [{ email: 'to@example.com' }] } }], + list: [{ + id: 'sub-1', + sendAt: ORIGINAL_AT, + envelope: { mailFrom: { email: 'me@example.com' }, rcptTo: [{ email: 'to@example.com' }] }, + }], }, '0'], ['Email/get', { list: [{ id: 'e-1', to: [{ email: 'to@example.com' }] }] }, '1'], + ['Mailbox/get', { list: [{ id: 'mb-drafts', role: 'drafts' }, { id: 'mb-sent', role: 'sent' }, { id: 'mb-in', role: 'inbox' }] }, '2'], ], }; const REPLACED = [['EmailSubmission/set', { @@ -60,8 +67,17 @@ const setCalls = () => mockRequest.mock.calls Object.keys(args.update ?? {}), args.destroy ?? [], ]); +/** The replacement create of the n-th EmailSubmission/set create call. */ +const createdArgs = (n = 0) => mockRequest.mock.calls + .map((c) => c[0][0]) + .filter(([method, args]: [string, { create?: object }]) => method === 'EmailSubmission/set' && args.create)[n][1]; +const REFUSED = (description: string) => set({ notCreated: { replacement: { type: 'forbiddenToSend', description } } }); describe('rescheduleScheduledSend', () => { + afterEach(() => { + mockRequest.mockReset(); + }); + it('keeps Cc, Bcc and the envelope sender of the held submission (B19)', async () => { mockRequest .mockResolvedValueOnce({ @@ -82,20 +98,18 @@ describe('rescheduleScheduledSend', () => { ['Email/get', { list: [{ id: 'e-1', to: [{ email: 'to@example.com' }], cc: [{ email: 'cc@example.com' }] }] }, '1'], ], }) - .mockResolvedValueOnce({ methodResponses: REPLACED }) .mockResolvedValueOnce(CANCELLED) - .mockResolvedValueOnce(DESTROYED('sub-1')); + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); const result = await rescheduleScheduledSend(SCHEDULED, 7200); expect(result).toEqual({ emailSubmissionId: 'sub-2', sendAt: '2026-09-25T08:00:00Z' }); - // What the server answered for each recipient comes with the envelope. const [lookup] = mockRequest.mock.calls[0][0]; expect(lookup).toEqual([ - 'EmailSubmission/get', { accountId: 'acc-1', ids: ['sub-1'], properties: ['envelope', 'deliveryStatus'] }, '0', + 'EmailSubmission/get', { accountId: 'acc-1', ids: ['sub-1'], properties: ['envelope', 'sendAt'] }, '0', ]); - const args = mockRequest.mock.calls[1][0][0][1]; - expect(args.create.replacement.envelope).toEqual({ + expect(createdArgs().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' }], }); @@ -116,104 +130,112 @@ describe('rescheduleScheduledSend', () => { }, '1'], ], }) - .mockResolvedValueOnce({ methodResponses: REPLACED }) .mockResolvedValueOnce(CANCELLED) - .mockResolvedValueOnce(DESTROYED('sub-1')); + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); await rescheduleScheduledSend(SCHEDULED, 60); - expect(mockRequest.mock.calls[1][0][0][1].create.replacement.envelope).toEqual({ + expect(createdArgs().create.replacement.envelope).toEqual({ mailFrom: { email: 'news@shop.example', parameters: { HOLDFOR: '60' } }, rcptTo: [{ email: 'to@example.com' }, { email: 'cc@example.com' }, { email: 'bcc@example.com' }], }); }); - it('creates the replacement before cancelling the original, then removes the canceled record', async () => { + // #1308: some servers refuse a second active submission for one Email, and + // some undo onSuccessUpdateEmail on cancel, pulling the message back out of + // Sent once the original went after its replacement. + it('cancels the original and removes its record before it creates the replacement, which files the message in Sent', async () => { mockRequest .mockResolvedValueOnce(LOOKUP) - .mockResolvedValueOnce({ methodResponses: REPLACED }) .mockResolvedValueOnce(CANCELLED) - .mockResolvedValueOnce(DESTROYED('sub-1')); + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); await rescheduleScheduledSend(SCHEDULED, 7200); - expect(setCalls()).toEqual([[['replacement'], [], []], [[], ['sub-1'], []], [[], [], ['sub-1']]]); - expect(mockRequest.mock.calls[2][0][0][1].update).toEqual({ 'sub-1': { undoStatus: 'canceled' } }); + expect(setCalls()).toEqual([[[], ['sub-1'], []], [[], [], ['sub-1']], [['replacement'], [], []]]); + expect(mockRequest.mock.calls[1][0][0][1].update).toEqual({ 'sub-1': { undoStatus: 'canceled' } }); + expect(createdArgs().onSuccessUpdateEmail).toEqual({ + '#replacement': { mailboxIds: { 'mb-sent': true }, 'keywords/$draft': null }, + }); }); - it('sends now as a 1-second hold, so the replacement can still be withdrawn', async () => { + it('sends now as a 1-second hold', async () => { mockRequest .mockResolvedValueOnce(LOOKUP) - .mockResolvedValueOnce({ methodResponses: REPLACED }) .mockResolvedValueOnce(CANCELLED) - .mockResolvedValueOnce(DESTROYED('sub-1')); + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce({ methodResponses: REPLACED }); await rescheduleScheduledSend(SCHEDULED, 0); - const replacement = mockRequest.mock.calls[1][0][0][1].create.replacement; - expect(replacement.envelope).toEqual({ + expect(createdArgs().create.replacement.envelope).toEqual({ mailFrom: { email: 'me@example.com', parameters: { HOLDFOR: '1' } }, rcptTo: [{ email: 'to@example.com' }], }); - expect(setCalls()).toEqual([[['replacement'], [], []], [[], ['sub-1'], []], [[], [], ['sub-1']]]); }); - it('leaves the original scheduled when the replacement is refused', async () => { + it('changes nothing when the original has already gone out', 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 } })) - .mockResolvedValueOnce(DESTROYED('sub-2')); + .mockResolvedValueOnce({ methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus: 'final' }] }, '0']] }); - await expect(rescheduleScheduledSend(SCHEDULED, 0)).rejects.toThrow('Failed to cancel the previous schedule: Already sent'); - // The original went out, so its record stays for the prune; the - // withdrawn replacement's goes at once. - expect(setCalls()).toEqual([ - [['replacement'], [], []], [[], ['sub-1'], []], [[], ['sub-2'], []], [[], [], ['sub-2']], - ]); - expect(warn).not.toHaveBeenCalled(); - warn.mockRestore(); + const err = await rescheduleScheduledSend(SCHEDULED, 0).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(NoLongerScheduledError); + expect(err).toMatchObject({ sent: true }); + // No replacement: it would send the message a second time. + expect(setCalls()).toEqual([[[], ['sub-1'], []]]); }); - it('still reports the failure when the replacement cannot be withdrawn either', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + it('changes nothing and reports the failure when the cancel fails while the original is still due', async () => { 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']] }); + .mockResolvedValueOnce({ methodResponses: [['EmailSubmission/get', { list: [{ id: 'sub-1', undoStatus: 'pending' }] }, '0']] }); - const result = await rescheduleScheduledSend(SCHEDULED, 7200); + await expect(rescheduleScheduledSend(SCHEDULED, 7200)).rejects.toThrow('Network request failed'); + expect(setCalls()).toEqual([[[], ['sub-1'], []]]); + }); - expect(result.emailSubmissionId).toBe('sub-2'); - // Unconfirmed, so the original's record is left for the prune. - expect(setCalls()).toEqual([[['replacement'], [], []], [[], ['sub-1'], []]]); + it('puts the original back at its old time, under a new id, when the replacement is refused', async () => { + vi.spyOn(Date, 'now').mockReturnValue(Date.parse(ORIGINAL_AT) - 1800_000); + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce(REFUSED('Quota exceeded')) + .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', restoredSendAt: ORIGINAL_AT, message: 'Quota exceeded' }); + // Half an hour was left of the original hold. + expect(createdArgs(1).create.replacement.envelope.mailFrom.parameters).toEqual({ HOLDFOR: '1800' }); + vi.mocked(Date.now).mockRestore(); + }); + + it('moves the message back to Drafts when neither the replacement nor the original can be made', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + mockRequest + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) + .mockResolvedValueOnce(REFUSED('Quota exceeded')) + .mockResolvedValueOnce(REFUSED('Quota exceeded')) + .mockResolvedValueOnce({ methodResponses: [['Email/set', { updated: { 'e-1': null } }, '0']] }); + + const err = await rescheduleScheduledSend(SCHEDULED, 7200).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(RescheduleLostError); + expect(err).toMatchObject({ inDrafts: true }); + const [, restore] = mockRequest.mock.calls.at(-1)![0][0]; + expect(restore.update).toEqual({ + 'e-1': { 'mailboxIds/mb-drafts': true, 'keywords/$draft': true, 'keywords/$seen': true, 'mailboxIds/mb-sent': null }, + }); }); }); @@ -392,24 +414,24 @@ describe('hold-limit rejections', () => { expect(jmapClient.learnHoldLimit).not.toHaveBeenCalled(); }); - it('turns a refused reschedule into ScheduleTooLateError', async () => { + it('turns a refused reschedule into ScheduleTooLateError, with the original put back', async () => { mockRequest - .mockResolvedValueOnce({ - methodResponses: [ - ['EmailSubmission/get', { list: [{ id: 'sub-1', envelope: { mailFrom: { email: 'me@example.com' }, rcptTo: [{ email: 'to@example.com' }] } }] }, '0'], - ['Email/get', { list: [] }, '1'], - ], - }) + .mockResolvedValueOnce(LOOKUP) + .mockResolvedValueOnce(CANCELLED) + .mockResolvedValueOnce(DESTROYED('sub-1')) .mockResolvedValueOnce({ methodResponses: [['EmailSubmission/set', { notCreated: { replacement: HOLD_REJECTED } }, '0']], - }); + }) + .mockResolvedValueOnce(set({ created: { replacement: { id: 'sub-3' } } })); const err = await rescheduleScheduledSend(SCHEDULED, 5 * 24 * 3600).catch((e) => e); - expect(err).toBeInstanceOf(ScheduleTooLateError); + expect(err).toBeInstanceOf(RescheduleRestoredError); + expect((err as RescheduleRestoredError).reason).toBeInstanceOf(ScheduleTooLateError); expect(jmapClient.learnHoldLimit).toHaveBeenCalledWith(172_800); - // The original stays scheduled. - expect(setCalls()).toEqual([[['replacement'], [], []]]); + // The original goes at its old time again, under a new id. + expect(setCalls()).toEqual([[[], ['sub-1'], []], [[], [], ['sub-1']], [['replacement'], [], []], [['replacement'], [], []]]); + mockRequest.mockReset(); }); }); @@ -491,9 +513,9 @@ describe('scheduled sends in shared accounts (webmail #874)', () => { mockRequest .mockResolvedValueOnce(LOOKUP) - .mockResolvedValueOnce(set({ created: { replacement: { id: 's-new', sendAt: FUTURE } } })) .mockResolvedValueOnce(set({ updated: { 's-grp': null } })) - .mockResolvedValueOnce(DESTROYED('s-grp')); + .mockResolvedValueOnce(DESTROYED('s-grp')) + .mockResolvedValueOnce(set({ created: { replacement: { id: 's-new', sendAt: FUTURE } } })); await rescheduleScheduledSend({ ...SCHEDULED, emailSubmissionId: 's-grp', accountId: 'grp-1' }, 0); expect(mockRequest.mock.calls.slice(2).map((c) => c[0][0][1].accountId)).toEqual(['grp-1', 'grp-1', 'grp-1', 'grp-1']); }); diff --git a/src/api/__tests__/fake-submission-server.ts b/src/api/__tests__/fake-submission-server.ts index a1cb75cc..b75dee07 100644 --- a/src/api/__tests__/fake-submission-server.ts +++ b/src/api/__tests__/fake-submission-server.ts @@ -389,7 +389,27 @@ export function createFakeSubmissionServer(options: { if (emails.delete(id)) (body.destroyed ??= []).push(id); else (body.notDestroyed ??= {})[id] = { type: 'notFound' }; } + // Pointer patches of mailboxes and keywords (a message back to Drafts). + for (const [id, patch] of Object.entries((args.update ?? {}) as Record>)) { + const email = emails.get(id); + if (!email) { + (body.notUpdated ??= {})[id] = { type: 'notFound' }; + continue; + } + for (const [path, value] of Object.entries(patch)) { + const [field, key] = path.split('/'); + const target = field === 'mailboxIds' ? email.mailboxIds : field === 'keywords' ? email.keywords : null; + if (!target || !key) continue; + if (value === null) delete target[key]; + else target[key] = true; + } + (body.updated ??= {})[id] = null; + } responses.push(['Email/set', body, callId]); + } else if (name === 'Mailbox/get') { + responses.push(['Mailbox/get', { + accountId, state: 'm1', list: [{ id: 'drafts', role: 'drafts' }, { id: 'sent', role: 'sent' }], notFound: [], + }, callId]); } else if (name === 'Email/import') { const body: Record = { accountId }; for (const [creationId, item] of Object.entries(args.emails as Record>)) { diff --git a/src/api/email.ts b/src/api/email.ts index 0126a54f..0e60768a 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -2144,21 +2144,53 @@ export async function cancelScheduledSend(emailSubmissionId: string, accountIdOv } } +/** + * A reschedule or "Send now" whose replacement the server refused, after the + * original had been cancelled: the original was put back at its old time + * under a new submission id, which the caller now holds instead (webmail + * RescheduleRestoredError). `reason` is why the replacement failed. + */ +export class RescheduleRestoredError extends Error { + constructor( + readonly reason: unknown, + readonly restoredSubmissionId: string, + readonly restoredSendAt?: string, + ) { + super(reason instanceof Error ? reason.message : String(reason)); + this.name = 'RescheduleRestoredError'; + } +} + +/** + * A reschedule or "Send now" that failed and could not put the original back + * either: the message is no longer scheduled. It was moved back to Drafts + * (`inDrafts`) so it does not sit in Sent looking sent. `reason` is why the + * replacement failed. + */ +export class RescheduleLostError extends Error { + constructor(readonly reason: unknown, readonly inDrafts: boolean) { + super(`Reschedule failed and the message is no longer scheduled: ${reason instanceof Error ? reason.message : String(reason)}`); + this.name = 'RescheduleLostError'; + } +} + /** * 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, and the - * recipients the server refused for the replacement when it took the others. - * When it refused the replacement a recipient the original had been taken - * for, the original is kept at its old time, and RecipientsRefusedError - * names whom the replacement would have lost. + * `holdForSeconds` = 0. The original is cancelled first and a replacement + * for the same Email, with the held envelope and a fresh HOLDFOR, created + * after it (#1308, webmail parity). The other order failed on servers that + * allow one active submission per Email and refuse the second create, and on + * servers that undo onSuccessUpdateEmail on cancel, which pulled the message + * back to Drafts once the original was cancelled after the replacement + * existed; the replacement now files it in Sent itself. + * + * A cancel that fails changes nothing: the original still goes out, or went + * 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. */ export async function rescheduleScheduledSend( scheduled: { @@ -2174,151 +2206,163 @@ export async function rescheduleScheduledSend( recipients?: EmailAddress[], ): Promise<{ emailSubmissionId?: string; sendAt?: string; refusedRecipients?: RefusedRecipient[] }> { const accountId = scheduled.accountId ?? jmapClient.accountId; - // "Send now" is a 1-second hold, so the replacement can still be withdrawn. + // "Send now" is a 1-second hold, as in the webmail. 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. What - // the server answered for each recipient when it took the original comes - // along: whom a refusal of the replacement costs the message depends on it. + // The Email's To/Cc/Bcc are the fallback when the server has none. The + // account's Drafts and Sent come along for filing the replacement and for + // a message that ends up not scheduled at all. const lookup = await jmapClient.request( [ ['EmailSubmission/get', { - accountId, ids: [scheduled.emailSubmissionId], properties: ['envelope', 'deliveryStatus'], + accountId, ids: [scheduled.emailSubmissionId], properties: ['envelope', 'sendAt'], }, '0'], ['Email/get', { accountId, ids: [scheduled.emailId], properties: ['to', 'cc', 'bcc'] }, '1'], + ['Mailbox/get', { accountId, ids: null, properties: ['id', 'role'] }, '2'], ], [CAPABILITIES.CORE, CAPABILITIES.MAIL, CAPABILITIES.SUBMISSION], ); const original = (requireMethodResult(lookup, '0', 'EmailSubmission/get').list as Array<{ envelope?: { mailFrom?: { email?: string }; rcptTo?: Array<{ email?: string }> } | null; - deliveryStatus?: Record | null; + sendAt?: string; }> | undefined)?.[0]; const envelope = original?.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(holdFor) }, - }, - rcptTo: rcpt, - }, + const rcptTo = source.map((r) => ({ email: (r.email ?? '').trim() })).filter((r) => r.email); + const mailFrom = envelope?.mailFrom?.email || scheduled.from?.[0]?.email; + // Without an answer the replacement is made without filing, and a message + // that can't be put back stays where it is. + const [mailboxAnswer, mailboxBody] = lookup.methodResponses.find(([, , callId]) => callId === '2') ?? []; + const roles = mailboxAnswer === 'Mailbox/get' + ? ((mailboxBody?.list ?? []) as Array<{ id: string; role?: string | null }>) + : []; + const draftsId = roles.find((m) => m.role === 'drafts')?.id; + const sentId = roles.find((m) => m.role === 'sent')?.id; + + type Created = { id: string; sendAt?: string; delivery: { recipients: RefusedRecipient[]; all: boolean } }; + // `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 + // after making room takes the time spent off it. + const holdSeconds = () => (until === undefined ? undefined : Math.max(1, Math.ceil((until - Date.now()) / 1000))); + const send = () => { + const seconds = holdSeconds(); + return jmapClient.request( + [ + ['EmailSubmission/set', { + accountId, + create: { + replacement: { + emailId: scheduled.emailId, + identityId: scheduled.identityId, + envelope: { + mailFrom: seconds === undefined + ? { email: mailFrom } + : { email: mailFrom, parameters: { HOLDFOR: String(seconds) } }, + rcptTo, + }, + }, + }, + ...(draftsId && sentId ? { + onSuccessUpdateEmail: { + '#replacement': { mailboxIds: { [sentId]: true }, [keywordPointer('$draft')]: null }, + }, + } : {}), + }, '0'], + deliveryStatusCall(accountId, 'replacement', '1'), + ], + SUBMISSION_USING, + ); + }; + let res = await send(); + // A full submission quota (Stalwart 1.0): make room and try once more. + if (submissionCreateError(res, 'replacement')?.type === 'overQuota') { + await makeRoomForSubmission(accountId); + res = await send(); + } + // Only the submission call counts: the implied Email/set of + // onSuccessUpdateEmail can fail while the submission was created. + const body = requireMethodResult(res, '0', 'EmailSubmission/set'); + const notCreated = body.notCreated?.replacement as { description?: string; type?: string } | undefined; + if (notCreated) throw submissionError(notCreated, 'Failed to reschedule', accountId); + const created = body.created?.replacement as { id?: string; sendAt?: string } | undefined; + if (!created?.id) throw new Error('Failed to reschedule: the server returned no submission'); + return { + id: created.id, + sendAt: created.sendAt, + delivery: refusedRecipients(res.methodResponses.find(([, , callId]) => callId === '1')), + }; + }; + + // A submission that took no recipient goes nowhere; its record only counts + // against the limit. The recipients it refused, or null when it took some. + const withdrawIfDead = async (created: Created): Promise => { + if (!created.delivery.all) return null; + if (isStalwartAccount(accountId)) await destroyEmailSubmissions([created.id], accountId); + return created.delivery.recipients; }; // HOLDFOR counts from when the server takes the request. - const sendAt = Date.now() + holdFor * 1000; - const createReplacement = (seconds: number) => jmapClient.request( - [ - ['EmailSubmission/set', { - accountId, - create: { - replacement: { - ...create, - envelope: { ...create.envelope, mailFrom: { ...create.envelope.mailFrom, parameters: { HOLDFOR: String(seconds) } } }, - }, - }, - }, '0'], - deliveryStatusCall(accountId, 'replacement', '1'), - ], - [CAPABILITIES.CORE, CAPABILITIES.SUBMISSION], - ); - let res = await createReplacement(holdFor); - // A full submission quota (Stalwart 1.0): make room and try once more. The - // original is still pending, so making room never touches it. - if (submissionCreateError(res, 'replacement')?.type === 'overQuota') { - await makeRoomForSubmission(accountId); - // Making room can take a while - a prune, a walk through the change log - - // and meanwhile the original may have gone out: its time came, or it was - // "Send now" of a message due any moment. A replacement would then send - // it a second time. A read that fails throws, and nothing changes. - const originalStatus = await readSubmissionUndoStatus(scheduled.emailSubmissionId, accountId); - if (originalStatus !== 'pending') throw new NoLongerScheduledError(originalStatus === 'final'); - // The time spent is taken off the hold; a moment already past goes out - // at once. - res = await createReplacement(Math.max(1, Math.ceil((sendAt - Date.now()) / 1000))); - } - const body = requireMethodResult(res, '0', 'EmailSubmission/set'); - const notCreated = body.notCreated?.replacement as { description?: string; type?: string } | undefined; - if (notCreated) throw submissionError(notCreated, 'Failed to reschedule', accountId); - const created = body.created?.replacement as { id?: string; sendAt?: string } | undefined; - if (!created?.id) throw new Error('Failed to reschedule: the server returned no submission'); + const holdUntil = Date.now() + holdFor * 1000; - // The recipients are checked again for the replacement, and a throttle - // used up since the original was taken ("452 4.4.5 Rate limit exceeded") - // can refuse some or all of them. Cancelling the original would lose the - // message for a recipient it took and the replacement didn't, so the - // original is kept then - it still goes at its old time to everyone it - // took - and the replacement is withdrawn. A recipient the original was - // refused for as well loses nothing, so the reschedule goes ahead; without - // a delivery status of the original, every recipient counts as taken. - // Should the replacement not be withdrawn, it goes ahead too, and the - // caller names the refused recipients: keeping both would send the others - // two copies (webmail aa99f038). - const delivery = refusedRecipients(res.methodResponses.find(([, , callId]) => callId === '1')); - const refusedBefore = new Set(Object.entries(original?.deliveryStatus ?? {}) - .filter(([, status]) => status?.delivered === 'no') - .map(([address]) => address.toLowerCase())); - const lost = delivery.recipients.filter((recipient) => !refusedBefore.has(recipient.email.toLowerCase())); - if (lost.length > 0 && await withdrawReplacement(created.id, accountId, delivery.all)) { - throw new RecipientsRefusedError(lost); - } - - // Either cancel below also removes the record it canceled, so a reschedule - // leaves only the replacement behind. + // Cancel first. Should it fail, nothing has changed and the original still + // goes out - unless it has gone out already ("Send now" of a message due + // any moment), or was cancelled on another device. Either cancel also + // removes the record it canceled. 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}`); + const status = await submissionUndoStatus(scheduled.emailSubmissionId, accountId); + if (status === 'final' || status === 'canceled') throw new NoLongerScheduledError(status === 'final'); + throw err; + } + + let replacement: Created; + try { + replacement = await createSubmission(holdUntil); + // The recipients are checked again for the replacement, and a throttle + // used up since the original was taken can refuse some or all of them. + // When it took nobody the reschedule failed; when it took some, the + // caller names the refused ones. + const refused = await withdrawIfDead(replacement); + if (refused) throw new RecipientsRefusedError(refused); + } catch (err) { + // 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; + let restored: Created | undefined; + 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); } + if (restored && !(await withdrawIfDead(restored).catch(() => null))) { + throw new RescheduleRestoredError(err, restored.id, restored.sendAt ?? original?.sendAt); + } + // Neither went through: the message is not scheduled any more. It goes + // back to Drafts rather than sit in Sent looking sent. + let inDrafts = false; + if (draftsId) { + try { + await restoreEmailToDraft(scheduled.emailId, draftsId, sentId, accountId); + inDrafts = true; + } catch (draftErr) { + console.warn('[email] could not move the unscheduled message back to Drafts:', draftErr); + } + } + throw new RescheduleLostError(err, inDrafts); } return { - emailSubmissionId: created.id, - sendAt: created.sendAt, - ...(delivery.recipients.length > 0 ? { refusedRecipients: delivery.recipients } : {}), + emailSubmissionId: replacement.id, + sendAt: replacement.sendAt, + ...(replacement.delivery.recipients.length > 0 ? { refusedRecipients: replacement.delivery.recipients } : {}), }; } -/** - * Withdraw the replacement of a reschedule that keeps its original instead; - * true once it can no longer go out. A replacement the server took for - * nobody (`tookNobody`) holds no mail, so only its record goes, on Stalwart - * (elsewhere the prune takes it once it is final). Any other is cancelled, - * which "Send now" may be too late for: its replacement is held for a second - * only. A cancel that failed counts when a read shows it went through with - * only its answer lost. - */ -async function withdrawReplacement(replacementId: string, accountId: string, tookNobody: boolean): Promise { - if (tookNobody) { - if (isStalwartAccount(accountId)) await destroyEmailSubmissions([replacementId], accountId); - return true; - } - try { - await cancelScheduledSend(replacementId, accountId); - return true; - } catch (err) { - if ((await submissionUndoStatus(replacementId, accountId)) === 'canceled') return true; - console.warn('[email] could not withdraw a replacement some recipients were refused for:', err); - return false; - } -} - /** * A submission's `undoStatus`; undefined when the server no longer has the * record. Throws when it can't be read. diff --git a/src/lib/compose-reschedule.ts b/src/lib/compose-reschedule.ts new file mode 100644 index 00000000..f9f2d3d2 --- /dev/null +++ b/src/lib/compose-reschedule.ts @@ -0,0 +1,57 @@ +// What a failed "Send now" or reschedule leaves behind, and how to say so. +// The original is cancelled before its replacement is made (#1308), so a +// failure can end three ways: nothing changed (the cancel failed), the +// original was put back under a new submission id (RescheduleRestoredError), +// or the message is no longer scheduled and back in Drafts +// (RescheduleLostError). The undo bar and the Scheduled list both follow +// the message to its new id, or drop it. + +import { RescheduleLostError, RescheduleRestoredError } from '../api/email'; +import { ScheduleTooLateError } from '../api/jmap-result'; +import type { TranslateFn } from '../stores/locale-store'; +import { rescheduleFailureNotice } from './send-failure-message'; + +/** The submission a failed reschedule put back, which now stands for the message; null when none did. */ +export function restoredSubmission(err: unknown): { emailSubmissionId: string; sendAt?: string } | null { + return err instanceof RescheduleRestoredError + ? { emailSubmissionId: err.restoredSubmissionId, sendAt: err.restoredSendAt } + : null; +} + +/** Whether the message is not scheduled any more after the failure. */ +export function rescheduleLostMessage(err: unknown): boolean { + return err instanceof RescheduleLostError; +} + +/** + * The alert or toast for a failed reschedule. `formatWhen` formats an ISO + * time for "still scheduled for …"; `scheduledAt` is the time it was + * scheduled for before. + */ +export function rescheduleErrorNotice( + err: unknown, + t: TranslateFn, + failTitle: string, + formatWhen?: (iso: string) => string, + scheduledAt?: string, +): { title: string; message: string } { + if (err instanceof RescheduleLostError) { + return { + title: failTitle, + message: err.inDrafts + ? t('scheduled.reschedule_lost_drafts', 'The message is no longer scheduled. It is back in Drafts, where you can send it again.') + : t('scheduled.reschedule_lost', 'The message is no longer scheduled. Check Drafts and Sent before sending it again.'), + }; + } + if (err instanceof RescheduleRestoredError) { + const when = err.restoredSendAt ?? scheduledAt; + if (err.reason instanceof ScheduleTooLateError) { + return { + title: t('email_composer.schedule_too_late_title', 'Too far ahead'), + message: t('email_composer.schedule_too_late_body', 'That is later than this server allows. Pick an earlier time.'), + }; + } + return rescheduleFailureNotice(err.reason, t, failTitle, when && formatWhen ? formatWhen(when) : undefined); + } + return rescheduleFailureNotice(err, t, failTitle, scheduledAt && formatWhen ? formatWhen(scheduledAt) : undefined); +} diff --git a/src/screens/ScheduledScreen.tsx b/src/screens/ScheduledScreen.tsx index 9b74e6d2..dd4115eb 100644 --- a/src/screens/ScheduledScreen.tsx +++ b/src/screens/ScheduledScreen.tsx @@ -30,6 +30,7 @@ import { spacing, typography, componentSizes, radius, type ThemePalette } from ' import { useColors } from '../theme/colors'; import { MODAL_ORIENTATIONS } from '../lib/modal-orientations'; import { isPendingSend, sameScheduledSend, scheduledSendKey } from '../lib/scheduled-send-key'; +import { rescheduleErrorNotice, rescheduleLostMessage, restoredSubmission } from '../lib/compose-reschedule'; type Props = NativeStackScreenProps; @@ -84,6 +85,26 @@ export default function ScheduledScreen({ navigation }: Props) { try { await action(); } catch (e) { + // A failed reschedule or "Send now" put the original back under a new + // id, or could not, and the message is back in Drafts (#1308). + const restored = restoredSubmission(e); + if (restored || rescheduleLostMessage(e)) { + if (restored) { + setItems((prev) => prev.map((i) => (sameScheduledSend(i, item) + ? { ...i, emailSubmissionId: restored.emailSubmissionId, sendAt: restored.sendAt ?? i.sendAt } + : i))); + const pending = useSendUndoStore.getState().pending; + if (isPendingSend(pending, item, jmapClient.accountId)) { + useSendUndoStore.getState().setPending({ ...pending!, emailSubmissionId: restored.emailSubmissionId }); + } + } else { + clearUndoFor(item); + removeItem(item); + } + const { title, message } = rescheduleErrorNotice(e, t, failTitle, formatWhen, item.sendAt); + Alert.alert(title, message); + return; + } if (e instanceof ScheduleTooLateError) { Alert.alert( t('email_composer.schedule_too_late_title', 'Too far ahead'), diff --git a/src/stores/__tests__/send-undo-store.test.ts b/src/stores/__tests__/send-undo-store.test.ts index 1a2a33e9..e2831d15 100644 --- a/src/stores/__tests__/send-undo-store.test.ts +++ b/src/stores/__tests__/send-undo-store.test.ts @@ -1,11 +1,17 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -vi.mock('../../api/email', () => ({ - cancelScheduledSend: vi.fn(async () => undefined), - rescheduleScheduledSend: vi.fn(async () => ({ emailSubmissionId: 'sub-2' })), - restoreEmailToDraft: vi.fn(async () => undefined), - getFullEmail: vi.fn(), -})); +// The error classes are the real ones: the store tells them apart. +vi.mock('../../api/email', async (importOriginal) => { + const { RescheduleLostError, RescheduleRestoredError } = await importOriginal(); + return { + RescheduleLostError, + RescheduleRestoredError, + cancelScheduledSend: vi.fn(async () => undefined), + rescheduleScheduledSend: vi.fn(async () => ({ emailSubmissionId: 'sub-2' })), + restoreEmailToDraft: vi.fn(async () => undefined), + getFullEmail: vi.fn(), + }; +}); // The login the client serves: me@example.com on mail.example.com, whose // own JMAP account is 'primary'. Tests switch it to another server. @@ -38,6 +44,7 @@ vi.mock('../../api/submissions', () => ({ })); import * as emailApi from '../../api/email'; +import { RescheduleLostError, RescheduleRestoredError } from '../../api/email'; import * as submissions from '../../api/submissions'; import { NoLongerScheduledError, RecipientsRefusedError, SubmissionQuotaError } from '../../api/jmap-result'; import type { Email, Mailbox } from '../../api/types'; @@ -221,12 +228,15 @@ describe('send-undo-store', () => { expect(prune).toHaveBeenCalledTimes(2); }); - it('explains a send now refused for a full submission quota (Stalwart 1.0)', async () => { + // The original is cancelled before the release is made (#1308): when the + // release is refused, the original is put back under a new id, which + // Undo cancels from then on. + it('explains a send now refused for a full submission quota (Stalwart 1.0), and follows the put-back original', async () => { vi.spyOn(console, 'warn').mockImplementation(() => undefined); - api.rescheduleScheduledSend.mockRejectedValueOnce(new SubmissionQuotaError( + api.rescheduleScheduledSend.mockRejectedValueOnce(new RescheduleRestoredError(new SubmissionQuotaError( 'There are too many email submissions, please delete some before adding a new one.', true, - )); + ), 'sub-9', '2026-09-24T10:00:10Z')); useSendUndoStore.getState().setPending(entry()); await expect(useSendUndoStore.getState().sendNow()).resolves.toBe(false); @@ -236,11 +246,13 @@ describe('send-undo-store', () => { title: 'Could not send now', message: expect.stringContaining('ask your administrator to raise the email submission limit'), }]); - // Still held: it goes out when the delay ends. - expect(useSendUndoStore.getState().pending).not.toBeNull(); + // Still held: it goes out when the delay ends, and Undo cancels the put-back one. + expect(useSendUndoStore.getState().pending).toMatchObject({ emailSubmissionId: 'sub-9', emailId: 'mail-1' }); + await useSendUndoStore.getState().undo(); + expect(api.cancelScheduledSend).toHaveBeenCalledWith('sub-9', undefined); }); - it('drops the entry when the message went out while send now made room (Stalwart 1.0)', async () => { + it('drops the entry when the message went out before send now could cancel it', async () => { vi.spyOn(console, 'warn').mockImplementation(() => undefined); api.rescheduleScheduledSend.mockRejectedValueOnce(new NoLongerScheduledError(true)); useSendUndoStore.getState().setPending(entry()); @@ -257,15 +269,14 @@ describe('send-undo-store', () => { }); // A throttle used up since the send was taken (Stalwart 0.16 and 1.0 check - // the recipients again for the release): the original was kept, and a + // the recipients again for the release): the original was put back, and a // "not sent" would invite sending it a second time. it('says the held message is still scheduled when the server refused a recipient of the release', async () => { vi.spyOn(console, 'warn').mockImplementation(() => undefined); - api.rescheduleScheduledSend.mockRejectedValueOnce(new RecipientsRefusedError([ + api.rescheduleScheduledSend.mockRejectedValueOnce(new RescheduleRestoredError(new RecipientsRefusedError([ { email: 'you@example.com', smtpReply: '452 4.4.5 Rate limit exceeded, try again later.' }, - ])); - const pending = entry(); - useSendUndoStore.getState().setPending(pending); + ]), 'sub-9')); + useSendUndoStore.getState().setPending(entry()); await expect(useSendUndoStore.getState().sendNow()).resolves.toBe(false); @@ -275,7 +286,22 @@ describe('send-undo-store', () => { message: 'Nothing was changed: the server refused you@example.com (452 4.4.5 Rate limit exceeded, try again later.).', }]); // It still goes out when the delay ends, and can still be undone. - expect(useSendUndoStore.getState()).toMatchObject({ pending, busy: false }); + expect(useSendUndoStore.getState()).toMatchObject({ pending: { emailSubmissionId: 'sub-9' }, busy: false }); + }); + + it('drops the entry and points to Drafts when neither the release nor the original could be made', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + api.rescheduleScheduledSend.mockRejectedValueOnce(new RescheduleLostError(new Error('Quota exceeded'), true)); + useSendUndoStore.getState().setPending(entry()); + + await expect(useSendUndoStore.getState().sendNow()).resolves.toBe(false); + + expect(useToastStore.getState().toasts).toMatchObject([{ + type: 'error', + title: 'Could not send now', + message: expect.stringContaining('back in Drafts'), + }]); + expect(useSendUndoStore.getState()).toMatchObject({ pending: null, busy: false }); }); it('names the recipients the server refused for the release while the others get it', async () => { diff --git a/src/stores/send-undo-store.ts b/src/stores/send-undo-store.ts index 1bf0b2e8..7b3b90ef 100644 --- a/src/stores/send-undo-store.ts +++ b/src/stores/send-undo-store.ts @@ -17,7 +17,8 @@ import { draftContextFromEmail } from '../lib/draft-context'; import { composerLoginMoved, servedLoginOf } from '../lib/draft-login'; import { mailboxesOfAccount } from '../lib/mailbox-tree'; import { useAccountStore } from './account-store'; -import { partlyRefusedWarning, rescheduleFailureNotice } from '../lib/send-failure-message'; +import { rescheduleErrorNotice, rescheduleLostMessage, restoredSubmission } from '../lib/compose-reschedule'; +import { partlyRefusedWarning } from '../lib/send-failure-message'; import type { ComposeDraftContext } from '../navigation/types'; import { t } from './locale-store'; import { toast } from './toast-store'; @@ -182,11 +183,22 @@ export const useSendUndoStore = create((set, get) => ({ // The message stays held and goes out when the delay ends; say why it // did not go now: a full submission quota, say, or recipients refused // for the release that the held send had been taken for. - const { title, message } = rescheduleFailureNotice(err, t, t('scheduled.send_now_failed', 'Could not send now')); + const { title, message } = rescheduleErrorNotice(err, t, t('scheduled.send_now_failed', 'Could not send now')); toast.error(title, message); - // Making room for the release took long enough for the message to go - // out, or be cancelled on another device: nothing is left to undo. - set(err instanceof NoLongerScheduledError ? { pending: null, busy: false } : { busy: false }); + // The original was cancelled first and put back under a new id: Undo + // cancels that one now. + const restored = restoredSubmission(err); + if (restored) { + set({ + pending: { ...entry, emailSubmissionId: restored.emailSubmissionId, sendAt: restored.sendAt ?? entry.sendAt }, + busy: false, + }); + return false; + } + // It went out, was cancelled on another device, or is no longer + // scheduled at all: nothing is left to undo. + const gone = err instanceof NoLongerScheduledError || rescheduleLostMessage(err); + set(gone ? { pending: null, busy: false } : { busy: false }); return false; } },