move "link or copy" functionality to moxio

and add a bit more logging for unexpected failures when closing files.
and make tests pass with a TMPDIR on a different filesystem than the testdata directory.

backup.go

@@ -240,15 +240,19 @@ func backupctl(ctx context.Context, ctl *ctl) {
 		if err != nil {
 			return false, fmt.Errorf("open source path %s: %v", srcpath, err)
 		}
-		defer sf.Close()
+		defer func() {
+			err := sf.Close()
+			ctl.log.Check(err, "closing copied source file")
+		}()
 
 		df, err := os.OpenFile(dstpath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0660)
 		if err != nil {
-			return false, fmt.Errorf("open destination path %s: %v", dstpath, err)
+			return false, fmt.Errorf("create destination path %s: %v", dstpath, err)
 		}
 		defer func() {
 			if df != nil {
-				df.Close()
+				err := df.Close()
+				ctl.log.Check(err, "closing partial destination file")
 			}
 		}()
 		if _, err := io.Copy(df, sf); err != nil {
@@ -257,7 +261,7 @@ func backupctl(ctx context.Context, ctl *ctl) {
 		err = df.Close()
 		df = nil
 		if err != nil {
-			return false, fmt.Errorf("close: %v", err)
+			return false, fmt.Errorf("closing destination file: %v", err)
 		}
 		return false, nil
 	}

http/account.go

@@ -35,7 +35,7 @@ var accountapiJSON []byte
 //go:embed account.html
 var accountHTML []byte
 
-var accountDoc = mustParseAPI(accountapiJSON)
+var accountDoc = mustParseAPI("account", accountapiJSON)
 
 var accountSherpaHandler http.Handler
 

http/admin.go

@@ -59,14 +59,14 @@ var adminapiJSON []byte
 //go:embed admin.html
 var adminHTML []byte
 
-var adminDoc = mustParseAPI(adminapiJSON)
+var adminDoc = mustParseAPI("admin", adminapiJSON)
 
 var adminSherpaHandler http.Handler
 
-func mustParseAPI(buf []byte) (doc sherpadoc.Section) {
+func mustParseAPI(api string, buf []byte) (doc sherpadoc.Section) {
 	err := json.Unmarshal(buf, &doc)
 	if err != nil {
-		xlog.Fatalx("parsing api docs", err)
+		xlog.Fatalx("parsing api docs", err, mlog.Field("api", api))
 	}
 	return doc
 }

imapserver/server.go

@@ -3065,15 +3065,25 @@ func (c *conn) cmdxCopy(isUID bool, tag, cmd string, p *parser) {
 			}
 
 			// Copy message files to new message ID's.
+			syncDirs := map[string]struct{}{}
 			for i := range origMsgIDs {
 				src := c.account.MessagePath(origMsgIDs[i])
 				dst := c.account.MessagePath(newMsgIDs[i])
-				os.MkdirAll(filepath.Dir(dst), 0770) // todo optimization: keep track of dirs we already created, don't create them again
+				dstdir := filepath.Dir(dst)
-				err := c.linkOrCopyFile(dst, src)
+				if _, ok := syncDirs[dstdir]; !ok {
+					os.MkdirAll(dstdir, 0770)
+					syncDirs[dstdir] = struct{}{}
+				}
+				err := moxio.LinkOrCopy(c.log, dst, src, nil, true)
 				xcheckf(err, "link or copy file %q to %q", src, dst)
 				createdIDs = append(createdIDs, newMsgIDs[i])
 			}
 
+			for dir := range syncDirs {
+				err := moxio.SyncDir(dir)
+				xcheckf(err, "sync directory")
+			}
+
 			err = c.account.RetrainMessages(context.TODO(), c.log, tx, nmsgs, false)
 			xcheckf(err, "train copied messages")
 		})
@@ -3095,49 +3105,6 @@ func (c *conn) cmdxCopy(isUID bool, tag, cmd string, p *parser) {
 	c.writeresultf("%s OK [COPYUID %d %s %s] copied", tag, mbDst.UIDValidity, compactUIDSet(origUIDs).String(), compactUIDSet(newUIDs).String())
 }
 
-func (c *conn) linkOrCopyFile(dst, src string) error {
-	// Try hardlink first.
-	if err := os.Link(src, dst); err == nil {
-		return nil
-	}
-
-	// File system may not support hardlinks, or link would cross file systems. Do a regular file copy.
-	sf, err := os.Open(src)
-	if err != nil {
-		return err
-	}
-	defer func() {
-		err := sf.Close()
-		c.xsanity(err, "closing copied src file")
-	}()
-
-	df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660)
-	if err != nil {
-		return err
-	}
-	defer func() {
-		if df != nil {
-			err = os.Remove(df.Name())
-			c.xsanity(err, "removing unfinished dst file")
-			err = df.Close()
-			c.xsanity(err, "closing unfinished dst file")
-		}
-	}()
-
-	if _, err := io.Copy(df, sf); err != nil {
-		return err
-	}
-	if err := df.Close(); err != nil {
-		xerr := os.Remove(df.Name())
-		c.xsanity(xerr, "removing unfinished dst file")
-		df = nil
-		return err
-	}
-	// todo: may need to do a file/dir sync to flush to disk. better to do it once after multiple linkOrCopyFile calls.
-	df = nil
-	return nil
-}
-
 // Move moves messages from the currently selected/active mailbox to a named mailbox.
 //
 // State: Selected

moxio/linkcopy.go

@@ -0,0 +1,71 @@
+package moxio
+
+import (
+	"fmt"
+	"io"
+	"os"
+
+	"github.com/mjl-/mox/mlog"
+)
+
+// LinkOrCopy attempts to make a hardlink dst. If that fails, it will try to do a
+// regular file copy. If srcReaderOpt is not nil, it will be used for reading. If
+// sync is true and the file is copied, Sync is called on the file after writing to
+// ensure the file is written on disk. Callers should also sync the directory of
+// the destination file, but may want to do that after linking/copying multiple
+// files. If dst was created and an error occurred, it is removed.
+func LinkOrCopy(log *mlog.Log, dst, src string, srcReaderOpt io.Reader, sync bool) (rerr error) {
+	// Try hardlink first.
+	err := os.Link(src, dst)
+	if err == nil {
+		return nil
+	} else if os.IsNotExist(err) {
+		// No point in trying with regular copy, we would fail again. Either src doesn't
+		// exist or dst directory doesn't exist.
+		return err
+	}
+
+	// File system may not support hardlinks, or link could be crossing file systems.
+	// Do a regular file copy.
+	if srcReaderOpt == nil {
+		sf, err := os.Open(src)
+		if err != nil {
+			return fmt.Errorf("open source file: %w", err)
+		}
+		defer func() {
+			err := sf.Close()
+			log.Check(err, "closing copied source file")
+		}()
+		srcReaderOpt = sf
+	}
+
+	df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660)
+	if err != nil {
+		return fmt.Errorf("create destination: %w", err)
+	}
+	defer func() {
+		if df != nil {
+			err = os.Remove(dst)
+			log.Check(err, "removing partial destination file")
+			err = df.Close()
+			log.Check(err, "closing partial destination file")
+		}
+	}()
+
+	if _, err := io.Copy(df, srcReaderOpt); err != nil {
+		return fmt.Errorf("copy: %w", err)
+	}
+	if sync {
+		if err := df.Sync(); err != nil {
+			return fmt.Errorf("sync destination: %w", err)
+		}
+	}
+	err = df.Close()
+	df = nil
+	if err != nil {
+		err := os.Remove(dst)
+		log.Check(err, "removing partial destination file")
+		return err
+	}
+	return nil
+}

moxio/linkcopy_test.go

@@ -0,0 +1,57 @@
+package moxio
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/mjl-/mox/mlog"
+)
+
+func tcheckf(t *testing.T, err error, format string, args ...any) {
+	if err != nil {
+		t.Helper()
+		t.Fatalf("%s: %s", fmt.Sprintf(format, args...), err)
+	}
+}
+
+func TestLinkOrCopy(t *testing.T) {
+	log := mlog.New("linkorcopy")
+
+	// link in same directory. file exists error. link to file in non-existent
+	// directory (exists error). link to file in system temp dir (hopefully other file
+	// system).
+	src := "linkorcopytest-src.txt"
+	f, err := os.Create(src)
+	tcheckf(t, err, "creating test file")
+	defer os.Remove(src)
+	err = LinkOrCopy(log, "linkorcopytest-dst.txt", src, nil, false)
+	tcheckf(t, err, "linking file")
+	err = os.Remove("linkorcopytest-dst.txt")
+	tcheckf(t, err, "remove dst")
+
+	err = LinkOrCopy(log, "bogus/linkorcopytest-dst.txt", src, nil, false)
+	if err == nil || !os.IsNotExist(err) {
+		t.Fatalf("expected is not exist, got %v", err)
+	}
+
+	// Try with copying the file. This can currently only really happen on systems that
+	// don't support hardlinking. Because other code and tests already use os.Rename on
+	// similar files, which will fail for being cross-filesystem (and we do want
+	// users/admins to have the mox temp dir on the same file system as the account
+	// files).
+	dst := filepath.Join(os.TempDir(), "linkorcopytest-dst.txt")
+	err = LinkOrCopy(log, dst, src, nil, true)
+	tcheckf(t, err, "copy file")
+	err = os.Remove(dst)
+	tcheckf(t, err, "removing dst")
+
+	// Copy based on open file.
+	_, err = f.Seek(0, 0)
+	tcheckf(t, err, "seek to start")
+	err = LinkOrCopy(log, dst, src, f, true)
+	tcheckf(t, err, "copy file from reader")
+	err = os.Remove(dst)
+	tcheckf(t, err, "removing dst")
+}

queue/queue.go

@@ -240,14 +240,9 @@ func Add(ctx context.Context, log *mlog.Log, senderAccount string, mailFrom, rcp
 			// Could be due to cross-filesystem rename. Users shouldn't configure their systems that way.
 			return 0, fmt.Errorf("move message into queue dir: %w", err)
 		}
-	} else if err := os.Link(msgFile.Name(), dst); err != nil {
+	} else if err := moxio.LinkOrCopy(log, dst, msgFile.Name(), nil, true); err != nil {
-		// Assume file system does not support hardlinks. Copy it instead.
+		return 0, fmt.Errorf("linking/copying message to new file: %s", err)
-		if err := writeFile(dst, &moxio.AtReader{R: msgFile}); err != nil {
+	} else if err := moxio.SyncDir(dstDir); err != nil {
-			return 0, fmt.Errorf("copying message to new file: %s", err)
-		}
-	}
-
-	if err := moxio.SyncDir(dstDir); err != nil {
 		return 0, fmt.Errorf("sync directory: %v", err)
 	}
 
@@ -261,29 +256,6 @@ func Add(ctx context.Context, log *mlog.Log, senderAccount string, mailFrom, rcp
 	return qm.ID, nil
 }
 
-// write contents of r to new file dst, for delivering a message.
-func writeFile(dst string, r io.Reader) error {
-	df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660)
-	if err != nil {
-		return fmt.Errorf("create: %w", err)
-	}
-	defer func() {
-		if df != nil {
-			err := df.Close()
-			xlog.Check(err, "closing file after failed write")
-		}
-	}()
-	if _, err := io.Copy(df, r); err != nil {
-		return fmt.Errorf("copy: %s", err)
-	} else if err := df.Sync(); err != nil {
-		return fmt.Errorf("sync: %s", err)
-	} else if err := df.Close(); err != nil {
-		return fmt.Errorf("close: %s", err)
-	}
-	df = nil
-	return nil
-}
-
 func formatIPDomain(d dns.IPDomain) string {
 	if len(d.IP) > 0 {
 		return "[" + d.IP.String() + "]"

queue/queue_test.go

@@ -491,20 +491,6 @@ func TestQueueStart(t *testing.T) {
 	time.Sleep(100 * time.Millisecond) // Racy... we won't get notified when work is done...
 }
 
-func TestWriteFile(t *testing.T) {
-	name := "../testdata/queue.test"
-	os.Remove(name)
-	defer os.Remove(name)
-	err := writeFile(name, strings.NewReader("test"))
-	if err != nil {
-		t.Fatalf("writeFile, unexpected error %v", err)
-	}
-	buf, err := os.ReadFile(name)
-	if err != nil || string(buf) != "test" {
-		t.Fatalf("writeFile, read file, got err %v, data %q", err, buf)
-	}
-}
-
 func TestGatherHosts(t *testing.T) {
 	mox.Context = ctxbg
 

store/account.go

@@ -693,13 +693,11 @@ func (a *Account) DeliverMessage(log *mlog.Log, tx *bstore.Tx, m *Message, msgFi
 
 	if consumeFile {
 		if err := os.Rename(msgFile.Name(), msgPath); err != nil {
+			// Could be due to cross-filesystem rename. Users shouldn't configure their systems that way.
 			return fmt.Errorf("moving msg file to destination directory: %w", err)
 		}
-	} else if err := os.Link(msgFile.Name(), msgPath); err != nil {
+	} else if err := moxio.LinkOrCopy(log, msgPath, msgFile.Name(), &moxio.AtReader{R: msgFile}, true); err != nil {
-		// Assume file system does not support hardlinks. Copy it instead.
+		return fmt.Errorf("linking/copying message to new file: %w", err)
-		if err := writeFile(msgPath, &moxio.AtReader{R: msgFile}); err != nil {
-			return fmt.Errorf("copying message to new file: %w", err)
-		}
 	}
 
 	if sync {
@@ -719,28 +717,6 @@ func (a *Account) DeliverMessage(log *mlog.Log, tx *bstore.Tx, m *Message, msgFi
 	return nil
 }
 
-// write contents of r to new file dst, for delivering a message.
-func writeFile(dst string, r io.Reader) error {
-	df, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0660)
-	if err != nil {
-		return fmt.Errorf("create: %w", err)
-	}
-	defer func() {
-		if df != nil {
-			df.Close()
-		}
-	}()
-	if _, err := io.Copy(df, r); err != nil {
-		return fmt.Errorf("copy: %s", err)
-	} else if err := df.Sync(); err != nil {
-		return fmt.Errorf("sync: %s", err)
-	} else if err := df.Close(); err != nil {
-		return fmt.Errorf("close: %s", err)
-	}
-	df = nil
-	return nil
-}
-
 // SetPassword saves a new password for this account. This password is used for
 // IMAP, SMTP (submission) sessions and the HTTP account web page.
 func (a *Account) SetPassword(password string) error {

store/account_test.go

@@ -231,20 +231,6 @@ func TestMailbox(t *testing.T) {
 	}
 }
 
-func TestWriteFile(t *testing.T) {
-	name := "../testdata/account.test"
-	os.Remove(name)
-	defer os.Remove(name)
-	err := writeFile(name, strings.NewReader("test"))
-	if err != nil {
-		t.Fatalf("writeFile, unexpected error %v", err)
-	}
-	buf, err := os.ReadFile(name)
-	if err != nil || string(buf) != "test" {
-		t.Fatalf("writeFile, read file, got err %v, data %q", err, buf)
-	}
-}
-
 func TestMessageRuleset(t *testing.T) {
 	f, err := os.Open("/dev/null")
 	tcheck(t, err, "open")

store/export_test.go

@@ -30,7 +30,7 @@ func TestExport(t *testing.T) {
 
 	log := mlog.New("export")
 
-	msgFile, err := os.CreateTemp("", "mox-test-export")
+	msgFile, err := CreateMessageTemp("mox-test-export")
 	tcheck(t, err, "create temp")
 	defer os.Remove(msgFile.Name()) // To be sure.
 	const msg = "test: test\r\n\r\ntest\r\n"

store/tmp.go

@@ -6,10 +6,12 @@ import (
 	"github.com/mjl-/mox/mox-"
 )
 
-// CreateMessageTemp creates a temporary file for a message to be delivered.
+// CreateMessageTemp creates a temporary file, e.g. for delivery. The is created in
-// Caller is responsible for removing the temporary file on error, and for closing the file.
+// subdirectory tmp of the data directory, so the file is on the same file system
-// Caller should ensure the contents of the file are synced to disk before
+// as the accounts directory, so renaming files can succeed. The caller is
-// attempting to deliver the message.
+// responsible for closing and possibly removing the file. The caller should ensure
+// the contents of the file are synced to disk before attempting to deliver the
+// message.
 func CreateMessageTemp(pattern string) (*os.File, error) {
 	dir := mox.DataDirPath("tmp")
 	os.MkdirAll(dir, 0770)