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
2026-10-31 00:06:17 +00:00 · 2026-09-24 19:10:43 +02:00
parent ebd8dacdfd
commit 7e598c5bc9
2 changed files with 177 additions and 52 deletions
@@ -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<string, unknown>) => ({ 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<string, unknown> }]) => [
+    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']);
  });
 });
@@ -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<string, unknown> = { 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<string | undefined> {
+  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;
+  }
 }

 /**