GODT-797 APPEND waits for EXPUNGE to prevent data loss when Outlook moves from Spam or Trash

internal/imap/mailbox.go

@@ -177,16 +177,11 @@ func (im *imapMailbox) Check() error {
 // Expunge permanently removes all messages that have the \Deleted flag set
 // from the currently selected mailbox.
 func (im *imapMailbox) Expunge() error {
-	// Wait for any APPENDS to finish in order to avoid data loss when
+	// See comment of appendExpungeLock.
-	// Outlook sends commands too quickly STORE \Deleted, APPEND, EXPUNGE,
+	if im.storeMailbox.LabelID() == pmapi.TrashLabel || im.storeMailbox.LabelID() == pmapi.SpamLabel {
-	// APPEND FINISHED:
+		im.user.appendExpungeLock.Lock()
-	//
+		defer im.user.appendExpungeLock.Unlock()
-	// Based on Outlook APPEND request we will not create new message but
+	}
-	// move the original to desired mailbox. If the message is currently
-	// in Trash or Spam and EXPUNGE happens before APPEND processing is
-	// finished the message is deleted from Proton instead of moved to
-	// the desired mailbox.
-	im.user.waitForAppend()
 
 	im.user.backend.updates.block(im.user.currentAddressLowercase, im.name, operationDeleteMessage)
 	defer im.user.backend.updates.unblock(im.user.currentAddressLowercase, im.name, operationDeleteMessage)
@@ -197,6 +192,12 @@ func (im *imapMailbox) Expunge() error {
 // UIDExpunge permanently removes messages that have the \Deleted flag set
 // and UID passed from SeqSet from the currently selected mailbox.
 func (im *imapMailbox) UIDExpunge(seqSet *imap.SeqSet) error {
+	// See comment of appendExpungeLock.
+	if im.storeMailbox.LabelID() == pmapi.TrashLabel || im.storeMailbox.LabelID() == pmapi.SpamLabel {
+		im.user.appendExpungeLock.Lock()
+		defer im.user.appendExpungeLock.Unlock()
+	}
+
 	im.user.backend.updates.block(im.user.currentAddressLowercase, im.name, operationDeleteMessage)
 	defer im.user.backend.updates.unblock(im.user.currentAddressLowercase, im.name, operationDeleteMessage)
 

internal/imap/mailbox_message.go

@@ -70,9 +70,6 @@ func (im *imapMailbox) CreateMessage(flags []string, date time.Time, body imap.L
 	// Called from go-imap in goroutines - we need to handle panics for each function.
 	defer im.panicHandler.HandlePanic()
 
-	im.user.appendStarted()
-	defer im.user.appendFinished()
-
 	m, _, _, readers, err := message.Parse(body)
 	if err != nil {
 		return err
@@ -154,6 +151,9 @@ func (im *imapMailbox) CreateMessage(flags []string, date time.Time, body imap.L
 		}
 	}
 
+	im.user.appendExpungeLock.Lock()
+	defer im.user.appendExpungeLock.Unlock()
+
 	// Avoid appending a message which is already on the server. Apply the
 	// new label instead. This always happens with Outlook (it uses APPEND
 	// instead of COPY).

internal/imap/user.go

@@ -41,7 +41,22 @@ type imapUser struct {
 
 	currentAddressLowercase string
 
-	appendInProcess sync.WaitGroup
+	// Some clients, for example Outlook, do MOVE by STORE \Deleted, APPEND,
+	// EXPUNGE where APPEN and EXPUNGE can go in parallel. Usual IMAP servers
+	// do not deduplicate messages and this it's not an issue, but for APPEND
+	// for PM means just assigning label. That would cause to assign label and
+	// then delete the message, or in other words cause data loss.
+	// go-imap does not call CreateMessage till it gets the whole message from
+	// IMAP client, therefore with big message, simple wait for APPEND before
+	// performing EXPUNGE is not enough. There has to be two-way lock. Only
+	// that way even if EXPUNGE is called few ms before APPEND and message
+	// is deleted, APPEND will not just assing label but creates the message
+	// again.
+	// The issue is only when moving message from folder which is causing
+	// real removal, so Trash and Spam. Those only need to use the lock to
+	// not cause huge slow down as EXPUNGE is implicitly called also after
+	// UNSELECT, CLOSE, or LOGOUT.
+	appendExpungeLock sync.Mutex
 }
 
 // newIMAPUser returns struct implementing go-imap/user interface.
@@ -251,15 +266,3 @@ func (iu *imapUser) CreateMessageLimit() *uint32 {
 	upload := uint32(maxUpload)
 	return &upload
 }
-
-func (iu *imapUser) appendStarted() {
-	iu.appendInProcess.Add(1)
-}
-
-func (iu *imapUser) appendFinished() {
-	iu.appendInProcess.Done()
-}
-
-func (iu *imapUser) waitForAppend() {
-	iu.appendInProcess.Wait()
-}