Cleanup temporary files created during IMAP APPEND command.

Since a recent change (likely since implementing MULTIAPPEND), the temporary
files weren't removed any more. When changing it, I must have had the wrong
mental model about the MessageAdd method, assuming it would remove the temp
file.

Noticed during tests.

Author: mjl-

imapserver/replace.go

@@ -166,7 +166,7 @@ func (c *conn) cmdxReplace(isUID bool, tag, cmd string, p *parser) {
 	}
 
 	var file *os.File
-	var newMsgPath string
+	var newID int64 // Delivered message ID, file removed on error.
 	var f io.Writer
 	var commit bool
 
@@ -186,17 +186,14 @@ func (c *conn) cmdxReplace(isUID bool, tag, cmd string, p *parser) {
 		var err error
 		file, err = store.CreateMessageTemp(c.log, "imap-replace")
 		xcheckf(err, "creating temp file for message")
-		newMsgPath = file.Name()
+		defer store.CloseRemoveTempFile(c.log, file, "temporary message file")
 		f = file
 
 		defer func() {
-			if file != nil {
-				err := file.Close()
-				c.xsanity(err, "close temporary file for replace")
-			}
-			if newMsgPath != "" && !commit {
-				err := os.Remove(newMsgPath)
-				c.xsanity(err, "remove temporary file for replace")
+			if !commit && newID != 0 {
+				p := c.account.MessagePath(newID)
+				err := os.Remove(p)
+				c.xsanity(err, "remove message file for replace after error")
 			}
 		}()
 	}
@@ -289,8 +286,7 @@ func (c *conn) cmdxReplace(isUID bool, tag, cmd string, p *parser) {
 
 			err = c.account.MessageAdd(c.log, tx, &mbDst, &nm, file, store.AddOpts{})
 			xcheckf(err, "delivering message")
-			// Update path to what is stored in the account. We may still have to clean it up on errors.
-			newMsgPath = c.account.MessagePath(nm.ID)
+			newID = nm.ID
 
 			changes = append(changes, nm.ChangeAddUID(), mbDst.ChangeCounts())
 			if nkeywords != len(mbDst.Keywords) {

imapserver/server.go

@@ -3275,24 +3275,19 @@ func (c *conn) cmdAppend(tag, cmd string, p *parser) {
 		time       time.Time
 
 		file *os.File // Message file we are appending. Can be nil if we are writing to a nopWriteCloser due to being over quota.
-		path string   // Path if an actual file, either a temporary file, or of the message file stored in the account.
 
 		mw *message.Writer
-		m  store.Message
+		m  store.Message // New message. Delivered file for m.ID is removed on error.
 	}
 
 	var appends []*appendMsg
 	var commit bool
 	defer func() {
 		for _, a := range appends {
-			if a.file != nil {
-				err := a.file.Close()
-				c.xsanity(err, "closing APPEND temporary file")
-			}
-
-			if !commit && a.path != "" {
-				err := os.Remove(a.path)
-				c.xsanity(err, "removing APPEND temporary file")
+			if !commit && a.m.ID != 0 {
+				p := c.account.MessagePath(a.m.ID)
+				err := os.Remove(p)
+				c.xsanity(err, "cleaning up temporary append file after error")
 			}
 		}
 	}()
@@ -3385,7 +3380,7 @@ func (c *conn) cmdAppend(tag, cmd string, p *parser) {
 			var err error
 			a.file, err = store.CreateMessageTemp(c.log, "imap-append")
 			xcheckf(err, "creating temp file for message")
-			a.path = a.file.Name()
+			defer store.CloseRemoveTempFile(c.log, a.file, "temporary message file")
 			f = a.file
 
 			c.writelinef("+ ")
@@ -3398,7 +3393,7 @@ func (c *conn) cmdAppend(tag, cmd string, p *parser) {
 				var err error
 				a.file, err = store.CreateMessageTemp(c.log, "imap-append")
 				xcheckf(err, "creating temp file for message")
-				a.path = a.file.Name()
+				defer store.CloseRemoveTempFile(c.log, a.file, "temporary message file")
 				f = a.file
 			}
 		}
@@ -3483,12 +3478,10 @@ func (c *conn) cmdAppend(tag, cmd string, p *parser) {
 				// todo: do a single junk training
 				err = c.account.MessageAdd(c.log, tx, &mb, &a.m, a.file, store.AddOpts{SkipDirSync: true})
 				xcheckf(err, "delivering message")
-				// Update path to what is stored in the account. We may still have to clean it up on errors.
-				a.path = c.account.MessagePath(a.m.ID)
 
 				changes = append(changes, a.m.ChangeAddUID())
 
-				msgDirs[filepath.Dir(a.path)] = struct{}{}
+				msgDirs[filepath.Dir(c.account.MessagePath(a.m.ID))] = struct{}{}
 			}
 
 			changes = append(changes, mb.ChangeCounts())

store/account.go

@@ -2043,6 +2043,11 @@ type AddOpts struct {
 
 // MessageAdd delivers a mail message to the account.
 //
+// The file is hardlinked or copied, the caller must clean up the original file. If
+// this call succeeds, but the database transaction with the change can't be
+// committed, the caller must clean up the delivered message file identified by
+// m.ID.
+//
 // If the message does not fit in the quota, an error with ErrOverQuota is returned
 // and the mailbox and message are unchanged and the transaction can continue. For
 // other errors, the caller must abort the transaction.