UBERF-10669: Fix email channel duplicates (#8996)

* UBERF-10669: Fix email channel duplicates

Signed-off-by: Artem Savchenko <armisav@gmail.com>

* Update services/mail/mail-common/src/__tests__/channel.test.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Artyom Savchenko <armisav@gmail.com>

* UBERF-10669: Trim email

Signed-off-by: Artem Savchenko <armisav@gmail.com>

---------

Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artyom Savchenko <armisav@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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', () => {

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<string, Ref<Doc>>()
 
   constructor (
@@ -40,17 +40,18 @@ export class ChannelCache {
   async getOrCreateChannel (
     spaceId: Ref<PersonSpace>,
     participants: PersonId[],
-    emailAccount: string,
+    email: string,
     owner: PersonId
   ): Promise<Ref<Doc> | 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<PersonSpace>, emailAccount: string): void {
+  clearCache (spaceId: Ref<PersonSpace>, email: string): void {
-    this.cache.delete(`${spaceId}:${emailAccount}`)
+    const normalizedEmail = normalizeEmail(email)
+    this.cache.delete(`${spaceId}:${normalizedEmail}`)
   }
 
   clearAllCache (): void {
@@ -73,29 +75,30 @@ export class ChannelCache {
   private async fetchOrCreateChannel (
     space: Ref<PersonSpace>,
     participants: PersonId[],
-    emailAccount: string,
+    email: string,
     personId: PersonId
   ): Promise<Ref<Doc> | 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<PersonSpace>,
     participants: PersonId[],
-    emailAccount: string,
+    email: string,
     personId: PersonId
   ): Promise<Ref<Doc> | 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()
+}