diff --git a/src/modules/account/account.service.spec.ts b/src/modules/account/account.service.spec.ts index 933f27c..fc9bc31 100644 --- a/src/modules/account/account.service.spec.ts +++ b/src/modules/account/account.service.spec.ts @@ -479,7 +479,9 @@ describe('AccountService', () => { await expect(service.provisionAccount(params)).rejects.toThrow( 'Stalwart down', ); - expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id); + expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id, { + force: true, + }); expect(addresses.createProviderLink).not.toHaveBeenCalled(); }); @@ -505,10 +507,12 @@ describe('AccountService', () => { await expect(service.provisionAccount(params)).rejects.toThrow('DB down'); expect(provider.deleteAccount).toHaveBeenCalledWith(params.address); - expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id); + expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id, { + force: true, + }); }); - it('when bucket creation fails, then deletes the principal and account (undo) and rethrows', async () => { + it('when bucket creation fails, then deletes the principal and hard-deletes the account (undo) and rethrows', async () => { const createdAccount = MailAccount.build( newMailAccountAttributes({ userId: params.userId, @@ -532,11 +536,43 @@ describe('AccountService', () => { 'Bridge down', ); expect(provider.deleteAccount).toHaveBeenCalledWith(params.address); - expect(addresses.deleteProviderLink).toHaveBeenCalledWith('addr-id'); - expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id); + expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id, { + force: true, + }); expect(accounts.setNetworkBucketId).not.toHaveBeenCalled(); }); + it('when the provider delete fails during rollback, then still hard-deletes the account', async () => { + const createdAccount = MailAccount.build( + newMailAccountAttributes({ + userId: params.userId, + addresses: [], + }), + ); + + domains.findByDomain.mockResolvedValue(domain); + addresses.findByAddress.mockResolvedValue(null); + accounts.findByUserId.mockResolvedValue(null); + accounts.create.mockResolvedValue(createdAccount); + addresses.create.mockResolvedValue('addr-id'); + provider.createAccount.mockResolvedValue({ + provider: 'stalwart', + externalId: params.address, + internalId: '42', + }); + bridge.createMailBucket.mockRejectedValue(new Error('Bridge down')); + provider.deleteAccount.mockRejectedValue( + new Error('Stalwart unreachable'), + ); + + await expect(service.provisionAccount(params)).rejects.toThrow( + 'Bridge down', + ); + expect(accounts.delete).toHaveBeenCalledWith(createdAccount.id, { + force: true, + }); + }); + it('when concurrent provisioning race occurs, then returns the existing account', async () => { const existingAccount = MailAccount.build( newMailAccountAttributes({ userId: params.userId }), @@ -778,7 +814,7 @@ describe('AccountService', () => { ); }); - it('when bucket creation fails, then rolls back principal, link, and address', async () => { + it('when bucket creation fails, then rolls back principal and hard-deletes the address', async () => { const account = MailAccount.build(newMailAccountAttributes()); const domain = MailDomain.build(newMailDomainAttributes()); const newAddr = 'new@example.com'; @@ -788,6 +824,11 @@ describe('AccountService', () => { domains.findByDomain.mockResolvedValue(domain); addresses.findByAddress.mockResolvedValue(null); addresses.create.mockResolvedValue(newAddressId); + provider.createAccount.mockResolvedValue({ + provider: 'stalwart', + externalId: newAddr, + internalId: '7', + }); bridge.createMailBucket.mockRejectedValue(new Error('Bridge down')); await expect( @@ -795,8 +836,9 @@ describe('AccountService', () => { ).rejects.toThrow('Bridge down'); expect(provider.deleteAccount).toHaveBeenCalledWith(newAddr); - expect(addresses.deleteProviderLink).toHaveBeenCalledWith(newAddressId); - expect(addresses.delete).toHaveBeenCalledWith(newAddressId); + expect(addresses.delete).toHaveBeenCalledWith(newAddressId, { + force: true, + }); expect(addresses.setNetworkBucketId).not.toHaveBeenCalled(); }); @@ -862,7 +904,9 @@ describe('AccountService', () => { ), ).rejects.toThrow('provider down'); - expect(addresses.delete).toHaveBeenCalledWith(newAddressId); + expect(addresses.delete).toHaveBeenCalledWith(newAddressId, { + force: true, + }); expect(addresses.createProviderLink).not.toHaveBeenCalled(); }); @@ -888,7 +932,9 @@ describe('AccountService', () => { ).rejects.toThrow('DB down'); expect(provider.deleteAccount).toHaveBeenCalledWith(newAddr); - expect(addresses.delete).toHaveBeenCalledWith(newAddressId); + expect(addresses.delete).toHaveBeenCalledWith(newAddressId, { + force: true, + }); }); }); diff --git a/src/modules/account/account.service.ts b/src/modules/account/account.service.ts index 5276ece..457282b 100644 --- a/src/modules/account/account.service.ts +++ b/src/modules/account/account.service.ts @@ -226,7 +226,7 @@ export class AccountService { password, }); } catch (error) { - await this.accounts.delete(account.id); + await this.rollbackAccount(account.id); throw error; } @@ -239,9 +239,7 @@ export class AccountService { }); await this.createNetworkBucket(params.userId, addressId); } catch (error) { - await this.provider.deleteAccount(created.externalId); - await this.addresses.deleteProviderLink(addressId); - await this.accounts.delete(account.id); + await this.rollbackAccount(account.id, created.externalId); throw error; } @@ -315,7 +313,7 @@ export class AccountService { password, }); } catch (error) { - await this.addresses.delete(newAddressId); + await this.rollbackAddress(newAddressId); throw error; } @@ -326,18 +324,9 @@ export class AccountService { externalId: created.externalId, providerInternalId: created.internalId, }); - } catch (error) { - await this.provider.deleteAccount(created.externalId); - await this.addresses.delete(newAddressId); - throw error; - } - - try { await this.createNetworkBucket(userId, newAddressId); } catch (error) { - await this.provider.deleteAccount(address); - await this.addresses.deleteProviderLink(newAddressId); - await this.addresses.delete(newAddressId); + await this.rollbackAddress(newAddressId, created.externalId); throw error; } @@ -433,6 +422,38 @@ export class AccountService { } } + private async rollbackAccount( + accountId: string, + providerExternalId?: string, + ): Promise { + if (providerExternalId) { + await this.tryDeleteProviderAccount(providerExternalId); + } + await this.accounts.delete(accountId, { force: true }); + } + + private async rollbackAddress( + addressId: string, + providerExternalId?: string, + ): Promise { + if (providerExternalId) { + await this.tryDeleteProviderAccount(providerExternalId); + } + await this.addresses.delete(addressId, { force: true }); + } + + private async tryDeleteProviderAccount( + providerExternalId: string, + ): Promise { + try { + await this.provider.deleteAccount(providerExternalId); + } catch (error) { + this.logger.warn( + `Rollback: failed to delete provider account '${providerExternalId}': ${(error as Error).message}`, + ); + } + } + private async createNetworkBucket( userUuid: string, addressId: string, diff --git a/src/modules/account/repositories/account.repository.ts b/src/modules/account/repositories/account.repository.ts index 50749af..762b14a 100644 --- a/src/modules/account/repositories/account.repository.ts +++ b/src/modules/account/repositories/account.repository.ts @@ -42,8 +42,8 @@ export class AccountRepository { return this.toDomain(model); } - async delete(id: string): Promise { - await this.accountModel.destroy({ where: { id } }); + async delete(id: string, options?: { force?: boolean }): Promise { + await this.accountModel.destroy({ where: { id }, force: options?.force }); } async setNetworkBucketId(id: string, networkBucketId: string): Promise { diff --git a/src/modules/account/repositories/address.repository.ts b/src/modules/account/repositories/address.repository.ts index ec11742..5620e7d 100644 --- a/src/modules/account/repositories/address.repository.ts +++ b/src/modules/account/repositories/address.repository.ts @@ -167,8 +167,8 @@ export class AddressRepository { return model.id; } - async delete(id: string): Promise { - await this.addressModel.destroy({ where: { id } }); + async delete(id: string, options?: { force?: boolean }): Promise { + await this.addressModel.destroy({ where: { id }, force: options?.force }); } async setNetworkBucketId(id: string, networkBucketId: string): Promise {