diff --git a/services/mail/mail-common/src/__tests__/channel.test.ts b/services/mail/mail-common/src/__tests__/channel.test.ts index 87736295a4..b35d202200 100644 --- a/services/mail/mail-common/src/__tests__/channel.test.ts +++ b/services/mail/mail-common/src/__tests__/channel.test.ts @@ -154,6 +154,82 @@ describe('ChannelCache', () => { // Verify the cache doesn't contain the failed lookup expect((channelCache as any).cache.has(`${spaceId}:${emailAccount}`)).toBe(false) }) + + it('should not create duplicate channels for email addresses with different case', async () => { + // Arrange + const existingChannelId = 'mixed-case-channel-id' + const lowerCaseEmail = 'mixedcase@example.com' + const upperCaseEmail = 'MixedCase@Example.com' + + // Mock first findOne to return null (channel doesn't exist yet) + // and second findOne to return the channel (after creation) + mockClient.findOne + .mockResolvedValueOnce(undefined) // First call: channel doesn't exist + .mockResolvedValueOnce({ + // Second call after creation with different case + _id: existingChannelId as any, + title: lowerCaseEmail + } as any) + + mockClient.createDoc.mockResolvedValueOnce(existingChannelId as any) + + // Act - First create a channel with lowercase email + const channelId1 = await channelCache.getOrCreateChannel(spaceId, participants, lowerCaseEmail, personId) + + // Clear cache to simulate a fresh lookup + channelCache.clearCache(spaceId, lowerCaseEmail) + + // Act - Then try to create with uppercase email + const channelId2 = await channelCache.getOrCreateChannel(spaceId, participants, upperCaseEmail, personId) + + // Assert + expect(mockClient.findOne).toHaveBeenNthCalledWith(1, mail.tag.MailChannel, { title: lowerCaseEmail }) + expect(mockClient.findOne).toHaveBeenNthCalledWith(2, mail.tag.MailChannel, { title: lowerCaseEmail }) + + // Should only create doc once + expect(mockClient.createDoc).toHaveBeenCalledTimes(1) + expect(mockClient.createMixin).toHaveBeenCalledTimes(1) + + // Both should return the same channel ID + expect(channelId1).toBe(existingChannelId) + expect(channelId2).toBe(existingChannelId) + expect(channelId1).toBe(channelId2) + }) + + it('should handle race conditions when creating channels', async () => { + // Arrange - Simulate a race condition where channel is created between mutex lock and double-check + const raceChannelId = 'race-condition-channel-id' + + // Mock behavior: + // 1. First findOne returns null (channel doesn't exist) + // 2. Second findOne (after mutex) returns a channel (someone else created it) + mockClient.findOne.mockResolvedValueOnce(undefined).mockResolvedValueOnce({ + _id: raceChannelId, + title: 'race@example.com' + } as any) + + // Act + const channelId = await channelCache.getOrCreateChannel(spaceId, participants, 'race@example.com', personId) + + // Assert + expect(mockClient.findOne).toHaveBeenCalledTimes(2) + expect(mockClient.createDoc).not.toHaveBeenCalled() // Should not create doc because of race check + expect(channelId).toBe(raceChannelId) + }) + + it('should normalize email addresses to lowercase before lookup and creation', async () => { + // Arrange - This test verifies that email normalization to lowercase is implemented + + const mixedCaseEmail = 'MiXeD@ExAmPlE.com' + + // Act + await channelCache.getOrCreateChannel(spaceId, participants, mixedCaseEmail, personId) + + // Assert - If email normalization is implemented, this would pass + expect(mockClient.findOne).toHaveBeenCalledWith(mail.tag.MailChannel, { + title: expect.stringMatching(/mixed@example\.com/i) + }) + }) }) describe('clearCache', () => { diff --git a/services/mail/mail-common/src/channel.ts b/services/mail/mail-common/src/channel.ts index 3a024308e6..b0405c46b5 100644 --- a/services/mail/mail-common/src/channel.ts +++ b/services/mail/mail-common/src/channel.ts @@ -25,7 +25,7 @@ const createMutex = new SyncMutex() * Caches channel references to reduce calls to create mail channels */ export class ChannelCache { - // Key is `${spaceId}:${emailAccount}` + // Key is `${spaceId}:${normalizedEmail}` private readonly cache = new Map>() constructor ( @@ -40,17 +40,18 @@ export class ChannelCache { async getOrCreateChannel ( spaceId: Ref, participants: PersonId[], - emailAccount: string, + email: string, owner: PersonId ): Promise | undefined> { - const cacheKey = `${spaceId}:${emailAccount}` + const normalizedEmail = normalizeEmail(email) + const cacheKey = `${spaceId}:${normalizedEmail}` let channel = this.cache.get(cacheKey) if (channel != null) { return channel } - channel = await this.fetchOrCreateChannel(spaceId, participants, emailAccount, owner) + channel = await this.fetchOrCreateChannel(spaceId, participants, normalizedEmail, owner) if (channel != null) { this.cache.set(cacheKey, channel) } @@ -58,8 +59,9 @@ export class ChannelCache { return channel } - clearCache (spaceId: Ref, emailAccount: string): void { - this.cache.delete(`${spaceId}:${emailAccount}`) + clearCache (spaceId: Ref, email: string): void { + const normalizedEmail = normalizeEmail(email) + this.cache.delete(`${spaceId}:${normalizedEmail}`) } clearAllCache (): void { @@ -73,29 +75,30 @@ export class ChannelCache { private async fetchOrCreateChannel ( space: Ref, participants: PersonId[], - emailAccount: string, + email: string, personId: PersonId ): Promise | undefined> { + const normalizedEmail = normalizeEmail(email) try { // First try to find existing channel - const channel = await this.client.findOne(mail.tag.MailChannel, { title: emailAccount }) + const channel = await this.client.findOne(mail.tag.MailChannel, { title: normalizedEmail }) if (channel != null) { - this.ctx.info('Using existing channel', { me: emailAccount, space, channel: channel._id }) + this.ctx.info('Using existing channel', { me: normalizedEmail, space, channel: channel._id }) return channel._id } - return await this.createNewChannel(space, participants, emailAccount, personId) + return await this.createNewChannel(space, participants, normalizedEmail, personId) } catch (err) { this.ctx.error('Failed to create channel', { - me: emailAccount, + me: normalizedEmail, space, workspace: this.workspace, error: err instanceof Error ? err.message : String(err) }) // Remove failed lookup from cache - this.cache.delete(`${space}:${emailAccount}`) + this.cache.delete(`${space}:${normalizedEmail}`) return undefined } @@ -104,18 +107,19 @@ export class ChannelCache { private async createNewChannel ( space: Ref, participants: PersonId[], - emailAccount: string, + email: string, personId: PersonId ): Promise | undefined> { - const mutexKey = `channel:${this.workspace}:${space}:${emailAccount}` + const normalizedEmail = normalizeEmail(email) + const mutexKey = `channel:${this.workspace}:${space}:${normalizedEmail}` const releaseLock = await createMutex.lock(mutexKey) try { // Double-check that channel doesn't exist after acquiring lock - const existingChannel = await this.client.findOne(mail.tag.MailChannel, { title: emailAccount }) + const existingChannel = await this.client.findOne(mail.tag.MailChannel, { title: normalizedEmail }) if (existingChannel != null) { this.ctx.info('Using existing channel (found after mutex lock)', { - me: emailAccount, + me: normalizedEmail, space, channel: existingChannel._id }) @@ -123,12 +127,12 @@ export class ChannelCache { } // Create new channel if it doesn't exist - this.ctx.info('Creating new channel', { me: emailAccount, space, personId }) + this.ctx.info('Creating new channel', { me: normalizedEmail, space, personId }) const channelId = await this.client.createDoc( chat.masterTag.Channel, space, { - title: emailAccount, + title: normalizedEmail, private: true, members: participants, archived: false, @@ -140,7 +144,7 @@ export class ChannelCache { personId ) - this.ctx.info('Creating mixin', { me: emailAccount, space, personId, channelId }) + this.ctx.info('Creating mixin', { me: normalizedEmail, space, personId, channelId }) await this.client.createMixin( channelId, chat.masterTag.Channel, @@ -187,3 +191,7 @@ export const ChannelCacheFactory = { return ChannelCacheFactory.instances.size } } + +function normalizeEmail (email: string): string { + return email.toLowerCase().trim() +}