From a05bae9f4ce67e3a691a836d4dcf5216ba4e610c Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Wed, 8 Jul 2026 17:27:36 -0600 Subject: [PATCH] feat(email): handle concurrent draft updates with conflict response - Introduced `DraftUpdateConflictError` to manage concurrent modifications of drafts. - Updated `EmailService` to throw a `ConflictException` when a draft is modified concurrently. - Enhanced `EmailController` to return a conflict response for concurrent draft updates. - Added unit tests to verify conflict handling in `EmailService` and `JmapMailProvider` during draft updates. --- src/modules/email/email.controller.ts | 4 + src/modules/email/email.service.spec.ts | 21 ++- src/modules/email/email.service.ts | 24 ++- src/modules/email/mail-provider.port.ts | 7 + .../jmap/jmap-mail.provider.spec.ts | 162 +++++++++++++++++- .../infrastructure/jmap/jmap-mail.provider.ts | 136 ++++++++++----- 6 files changed, 302 insertions(+), 52 deletions(-) diff --git a/src/modules/email/email.controller.ts b/src/modules/email/email.controller.ts index b17b23c..a7fcc02 100644 --- a/src/modules/email/email.controller.ts +++ b/src/modules/email/email.controller.ts @@ -21,6 +21,7 @@ import { ApiBadRequestResponse, ApiBearerAuth, ApiBody, + ApiConflictResponse, ApiNoContentResponse, ApiNotFoundResponse, ApiOkResponse, @@ -265,6 +266,9 @@ export class EmailController { description: 'Draft updated successfully', }) @ApiNotFoundResponse({ description: 'Draft not found' }) + @ApiConflictResponse({ + description: 'Draft was modified concurrently; retry the save', + }) updateDraft( @MailAddress('address') email: string, @Param('id') draftId: string, diff --git a/src/modules/email/email.service.spec.ts b/src/modules/email/email.service.spec.ts index 72814fe..3205c6c 100644 --- a/src/modules/email/email.service.spec.ts +++ b/src/modules/email/email.service.spec.ts @@ -1,12 +1,19 @@ /* eslint-disable @typescript-eslint/no-unsafe-assignment */ import { describe, it, test, expect, beforeEach, vi } from 'vitest'; import { Test } from '@nestjs/testing'; -import { NotFoundException, BadRequestException } from '@nestjs/common'; +import { + NotFoundException, + BadRequestException, + ConflictException, +} from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { createMock, type DeepMocked } from '@golevelup/ts-vitest'; import { Readable } from 'node:stream'; import { EmailService } from './email.service.js'; -import { MailProvider } from './mail-provider.port.js'; +import { + DraftUpdateConflictError, + MailProvider, +} from './mail-provider.port.js'; import { AccountService } from '../account/account.service.js'; import { BridgeClient } from '../infrastructure/bridge/bridge.service.js'; import { StalwartSmtpService } from '../infrastructure/smtp/stalwart-smtp.service.js'; @@ -680,6 +687,16 @@ describe('EmailService', () => { service.updateDraft(userEmail, 'missing-draft', newDraftEmailDto()), ).rejects.toThrow(NotFoundException); }); + + test('When the draft was modified concurrently, then the caller gets a conflict so it can retry the save', async () => { + provider.updateDraft.mockRejectedValue( + new DraftUpdateConflictError('draft-id'), + ); + + await expect( + service.updateDraft(userEmail, 'draft-id', newDraftEmailDto()), + ).rejects.toThrow(ConflictException); + }); }); describe('Discard Draft', () => { diff --git a/src/modules/email/email.service.ts b/src/modules/email/email.service.ts index f5514cb..bf4c715 100644 --- a/src/modules/email/email.service.ts +++ b/src/modules/email/email.service.ts @@ -1,5 +1,6 @@ import { BadRequestException, + ConflictException, Injectable, Logger, NotFoundException, @@ -7,7 +8,10 @@ import { import { ConfigService } from '@nestjs/config'; import { AccountService } from '../account/account.service.js'; import { BridgeClient } from '../infrastructure/bridge/bridge.service.js'; -import { MailProvider } from './mail-provider.port.js'; +import { + DraftUpdateConflictError, + MailProvider, +} from './mail-provider.port.js'; import type { DraftEmailDto, Email, @@ -299,11 +303,19 @@ export class EmailService { draftId: string, dto: DraftEmailDto, ): Promise { - const result = await this.mail.updateDraft( - userEmail, - draftId, - this.packDraftEnvelope(dto), - ); + let result: Email | null; + try { + result = await this.mail.updateDraft( + userEmail, + draftId, + this.packDraftEnvelope(dto), + ); + } catch (error) { + if (error instanceof DraftUpdateConflictError) { + throw new ConflictException(error.message); + } + throw error; + } if (!result) { throw new NotFoundException(`Draft ${draftId} not found`); } diff --git a/src/modules/email/mail-provider.port.ts b/src/modules/email/mail-provider.port.ts index ac4cb4d..cd2e799 100644 --- a/src/modules/email/mail-provider.port.ts +++ b/src/modules/email/mail-provider.port.ts @@ -18,6 +18,13 @@ import type { ThreadingHeaders, } from './email.types.js'; +export class DraftUpdateConflictError extends Error { + constructor(draftId: string) { + super(`Draft ${draftId} was modified concurrently, retry the save`); + this.name = 'DraftUpdateConflictError'; + } +} + export abstract class MailProvider { abstract getMailboxes(userEmail: string): Promise; abstract listEmails(params: ListEmails): Promise; diff --git a/src/modules/infrastructure/jmap/jmap-mail.provider.spec.ts b/src/modules/infrastructure/jmap/jmap-mail.provider.spec.ts index 0f846f3..a343185 100644 --- a/src/modules/infrastructure/jmap/jmap-mail.provider.spec.ts +++ b/src/modules/infrastructure/jmap/jmap-mail.provider.spec.ts @@ -3,7 +3,8 @@ import { Readable } from 'node:stream'; import { Test, type TestingModule } from '@nestjs/testing'; import { createMock, type DeepMocked } from '@golevelup/ts-vitest'; import { JmapMailProvider } from './jmap-mail.provider.js'; -import { JmapService } from './jmap.service.js'; +import { JmapError, JmapService } from './jmap.service.js'; +import { DraftUpdateConflictError } from '../../email/mail-provider.port.js'; import { newJmapMailbox, newJmapEmail, @@ -94,7 +95,11 @@ describe('JmapMailProvider', () => { jmapMultiResponse( { ids: [rep.id], total: 1 }, { list: [rep] }, - { list: [{ id: 'thread-1', emailIds: [earlier.id, rep.id, reply.id] }] }, + { + list: [ + { id: 'thread-1', emailIds: [earlier.id, rep.id, reply.id] }, + ], + }, { list: [earlier, rep, reply] }, ), ); @@ -524,6 +529,155 @@ describe('JmapMailProvider', () => { expect(result).not.toBeNull(); expect(result!.id).toBe(updatedDraft.id); }); + + test('When the draft is updated, then the destroy+create is guarded with ifInState from the prior read', async () => { + const draftsMailbox = newJmapMailbox({ role: 'drafts' }); + const identity = newJmapIdentity(); + const existingDraft = newJmapEmail({ keywords: { $draft: true } }); + const updatedDraft = newJmapEmail({ keywords: { $draft: true } }); + + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [identity] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [draftsMailbox] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [existingDraft], state: 'email-state-1' }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ created: { draft: { id: updatedDraft.id } } }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [updatedDraft] }), + ); + + await provider.updateDraft( + 'user@test.com', + existingDraft.id, + newDraftEmailDto(), + ); + + const setCall = jmapService.request.mock.calls.find( + (call) => call[1][0]![0] === 'Email/set', + )!; + const [, setArgs] = setCall[1][0]!; + expect(setArgs['ifInState']).toBe('email-state-1'); + expect(setArgs['destroy']).toEqual([existingDraft.id]); + }); + + test('When the account state changes between the read and the write, then the update is retried and succeeds', async () => { + const draftsMailbox = newJmapMailbox({ role: 'drafts' }); + const identity = newJmapIdentity(); + const existingDraft = newJmapEmail({ keywords: { $draft: true } }); + const updatedDraft = newJmapEmail({ keywords: { $draft: true } }); + const stateMismatch = new JmapError('JMAP method error', [ + ['error', { type: 'stateMismatch' }, 'r0'], + ]); + + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [identity] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [draftsMailbox] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [existingDraft], state: 'email-state-1' }), + ); + jmapService.request.mockRejectedValueOnce(stateMismatch); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [existingDraft], state: 'email-state-2' }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ created: { draft: { id: updatedDraft.id } } }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [updatedDraft] }), + ); + + const result = await provider.updateDraft( + 'user@test.com', + existingDraft.id, + newDraftEmailDto(), + ); + + expect(result!.id).toBe(updatedDraft.id); + const setCalls = jmapService.request.mock.calls.filter( + (call) => call[1][0]![0] === 'Email/set', + ); + expect(setCalls).toHaveLength(2); + expect(setCalls[1]![1][0]![1]['ifInState']).toBe('email-state-2'); + }); + + test('When the update keeps losing the state race, then a conflict error is surfaced', async () => { + const draftsMailbox = newJmapMailbox({ role: 'drafts' }); + const identity = newJmapIdentity(); + const existingDraft = newJmapEmail({ keywords: { $draft: true } }); + const stateMismatch = new JmapError('JMAP method error', [ + ['error', { type: 'stateMismatch' }, 'r0'], + ]); + + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [identity] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [draftsMailbox] }), + ); + for (let i = 0; i < 3; i++) { + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [existingDraft], state: `email-state-${i}` }), + ); + jmapService.request.mockRejectedValueOnce(stateMismatch); + } + + await expect( + provider.updateDraft( + 'user@test.com', + existingDraft.id, + newDraftEmailDto(), + ), + ).rejects.toThrow(DraftUpdateConflictError); + }); + + test('When the old draft cannot be destroyed but the copy was created, then the copy is cleaned up and the update fails', async () => { + const draftsMailbox = newJmapMailbox({ role: 'drafts' }); + const identity = newJmapIdentity(); + const existingDraft = newJmapEmail({ keywords: { $draft: true } }); + + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [identity] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [draftsMailbox] }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ list: [existingDraft], state: 'email-state-1' }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ + created: { draft: { id: 'orphan-copy' } }, + notDestroyed: { + [existingDraft.id]: { type: 'notFound', description: 'gone' }, + }, + }), + ); + jmapService.request.mockResolvedValueOnce( + jmapResponse({ destroyed: ['orphan-copy'] }), + ); + + await expect( + provider.updateDraft( + 'user@test.com', + existingDraft.id, + newDraftEmailDto(), + ), + ).rejects.toThrow('Failed to update draft: gone'); + + const lastCall = jmapService.request.mock.calls.at(-1)!; + const [methodName, methodArgs] = lastCall[1][0]!; + expect(methodName).toBe('Email/set'); + expect(methodArgs['destroy']).toEqual(['orphan-copy']); + }); }); describe('Discarding Draft', () => { @@ -1121,9 +1275,7 @@ describe('JmapMailProvider', () => { jmapMultiResponse( { list: [{ id: theirMessage.id, threadId: 'thread-1' }] }, { - list: [ - { id: 'thread-1', emailIds: [myReply.id, theirMessage.id] }, - ], + list: [{ id: 'thread-1', emailIds: [myReply.id, theirMessage.id] }], }, { list: [myReply, theirMessage] }, ), diff --git a/src/modules/infrastructure/jmap/jmap-mail.provider.ts b/src/modules/infrastructure/jmap/jmap-mail.provider.ts index 825949e..f8120ec 100644 --- a/src/modules/infrastructure/jmap/jmap-mail.provider.ts +++ b/src/modules/infrastructure/jmap/jmap-mail.provider.ts @@ -1,5 +1,8 @@ import { Injectable, Logger } from '@nestjs/common'; -import { MailProvider } from '../../email/mail-provider.port.js'; +import { + DraftUpdateConflictError, + MailProvider, +} from '../../email/mail-provider.port.js'; import type { DeleteEmailResult, DraftEmailDto, @@ -15,7 +18,11 @@ import type { ThreadingHeaders, } from '../../email/email.types.js'; import { decodeStalwartIdBig } from '../stalwart/stalwart-id.codec.js'; -import { JMAP_QUOTA_CAPABILITIES, JmapService } from './jmap.service.js'; +import { + JMAP_QUOTA_CAPABILITIES, + JmapError, + JmapService, +} from './jmap.service.js'; import type { DownloadAttachmentPayload, DownloadAttachmentResponse, @@ -72,6 +79,17 @@ interface TimedCache { const CACHE_TTL_MS = 60_000; +const UPDATE_DRAFT_MAX_ATTEMPTS = 3; + +const isStateMismatchError = (err: unknown): boolean => + err instanceof JmapError && + Array.isArray(err.details) && + err.details.some( + (invocation) => + Array.isArray(invocation) && + (invocation[1] as { type?: string } | null)?.type === 'stateMismatch', + ); + @Injectable() export class JmapMailProvider extends MailProvider { private readonly logger = new Logger(JmapMailProvider.name); @@ -649,52 +667,92 @@ export class JmapMailProvider extends MailProvider { email: identity.email, }); - const existingDraft = await this.getDraft(userEmail, draftId); - - if (!existingDraft) { - return null; - } - - const setResponse = await this.jmap.request>( - userEmail, - [ + for (let attempt = 1; attempt <= UPDATE_DRAFT_MAX_ATTEMPTS; attempt++) { + const getResponse = await this.jmap.request>( + userEmail, [ - 'Email/set', - { - accountId, - destroy: [draftId], - create: { draft: emailCreate }, - }, - 'r0', + [ + 'Email/get', + { accountId, ids: [draftId], properties: ['id', 'keywords'] }, + 'r0', + ], ], - ], - ); + ); - const setResult = setResponse.methodResponses[0]![1]; + const getResult = getResponse.methodResponses[0]![1]; + const existing = getResult.list[0]; + if (!existing?.keywords?.['$draft']) { + return null; + } - if (setResult.notDestroyed?.[draftId]) { - throw new Error( - `Failed to update draft: ${setResult.notDestroyed[draftId].description}`, - ); - } + let setResult: JmapSetResponse; + try { + const setResponse = await this.jmap.request>( + userEmail, + [ + [ + 'Email/set', + { + accountId, + ifInState: getResult.state, + destroy: [draftId], + create: { draft: emailCreate }, + }, + 'r0', + ], + ], + ); + setResult = setResponse.methodResponses[0]![1]; + } catch (err) { + if (isStateMismatchError(err)) { + if (attempt < UPDATE_DRAFT_MAX_ATTEMPTS) continue; + throw new DraftUpdateConflictError(draftId); + } + throw err; + } - if (setResult.notCreated?.['draft']) { - throw new Error( - `Failed to update draft: ${setResult.notCreated['draft'].description}`, - ); - } + const createdId = setResult.created?.['draft']?.id; + + if (setResult.notDestroyed?.[draftId]) { + if (createdId) { + await this.jmap + .request(userEmail, [ + ['Email/set', { accountId, destroy: [createdId] }, 'r0'], + ]) + .catch((cleanupErr) => { + this.logger.warn( + `Failed to clean up recreated draft ${createdId} after a partial update: ${ + cleanupErr instanceof Error + ? cleanupErr.message + : String(cleanupErr) + }`, + ); + }); + } + throw new Error( + `Failed to update draft: ${setResult.notDestroyed[draftId].description}`, + ); + } - const createdId = setResult.created?.['draft']?.id; - if (!createdId) { - throw new Error('Failed to recreate draft after destroy'); - } + if (setResult.notCreated?.['draft']) { + throw new Error( + `Failed to update draft: ${setResult.notCreated['draft'].description}`, + ); + } + + if (!createdId) { + throw new Error('Failed to recreate draft after destroy'); + } + + const updatedDraft = await this.getEmail(userEmail, createdId); + if (!updatedDraft) { + throw new Error('Failed to fetch the updated draft'); + } - const updatedDraft = await this.getEmail(userEmail, createdId); - if (!updatedDraft) { - throw new Error('Failed to fetch the updated draft'); + return updatedDraft; } - return updatedDraft; + throw new DraftUpdateConflictError(draftId); } async discardDraft(userEmail: string, id: string): Promise {