Close resource and info fork writers after upload

UploadHandler and UploadFolderHandler opened the resource and info fork
writers but only ever closed the data fork writer. On the OS-backed file
store this leaked a descriptor per uploaded file; on a backend that only
commits on Close (such as an object store, modeled by MemFileStore) the fork
data was silently dropped entirely.

Track the fork writers as closers and close them after the transfer completes,
before renaming the data fork into place. Add a MemFileStore-backed regression
test that fails without the fix.

2 files changed, 59 insertions, 10 deletions

diff --git a/hotline/file_transfer.go b/hotline/file_transfer.go
index 1526e28..685d0d5 100644
--- a/hotline/file_transfer.go
+++ b/hotline/file_transfer.go
@@ -330,18 +330,22 @@ func UploadHandler(rwc io.ReadWriter, fullPath string, fileTransfer *FileTransfe
 
 	rLogger.Debug("File upload started", "dstFile", fullPath)
 
-	rForkWriter := io.Discard
-	iForkWriter := io.Discard
+	var rForkWriter, iForkWriter io.Writer = io.Discard, io.Discard
+	var forkClosers []io.Closer
 	if preserveForks {
-		rForkWriter, err = f.rsrcForkWriter()
+		rFork, err := f.rsrcForkWriter()
 		if err != nil {
 			return err
 		}
+		rForkWriter = rFork
+		forkClosers = append(forkClosers, rFork)
 
-		iForkWriter, err = f.InfoForkWriter()
+		iFork, err := f.InfoForkWriter()
 		if err != nil {
 			return err
 		}
+		iForkWriter = iFork
+		forkClosers = append(forkClosers, iFork)
 	}
 
 	if err := receiveFile(rwc, file, rForkWriter, iForkWriter, fileTransfer.bytesSentCounter); err != nil {
@@ -349,10 +353,17 @@ func UploadHandler(rwc io.ReadWriter, fullPath string, fileTransfer *FileTransfe
 		return fmt.Errorf("receive file: %v", err)
 	}
 
-	// Close the file before attempting to rename it.
+	// Close the data fork and the resource/info fork writers before renaming.
+	// Closing the fork writers is required for backends that only commit on
+	// Close (e.g. an object store); leaving them open also leaks descriptors.
 	if err := file.Close(); err != nil {
 		return fmt.Errorf("close file: %v", err)
 	}
+	for _, c := range forkClosers {
+		if err := c.Close(); err != nil {
+			return fmt.Errorf("close fork: %v", err)
+		}
+	}
 
 	// Rename the temporary upload file to the final file name.
 	if err := fileStore.Rename(fullPath+".incomplete", fullPath); err != nil {
@@ -653,27 +664,38 @@ func UploadFolderHandler(rwc io.ReadWriter, fullPath string, fileTransfer *FileT
 					return err
 				}
 
-				rForkWriter := io.Discard
-				iForkWriter := io.Discard
+				var rForkWriter, iForkWriter io.Writer = io.Discard, io.Discard
+				var forkClosers []io.Closer
 				if preserveForks {
-					iForkWriter, err = hlFile.InfoForkWriter()
+					iFork, err := hlFile.InfoForkWriter()
 					if err != nil {
 						return err
 					}
+					iForkWriter = iFork
+					forkClosers = append(forkClosers, iFork)
 
-					rForkWriter, err = hlFile.rsrcForkWriter()
+					rFork, err := hlFile.rsrcForkWriter()
 					if err != nil {
 						return err
 					}
+					rForkWriter = rFork
+					forkClosers = append(forkClosers, rFork)
 				}
 				if err := receiveFile(rwc, incWriter, rForkWriter, iForkWriter, fileTransfer.bytesSentCounter); err != nil {
 					return err
 				}
 
-				// Close the file before attempting to rename it.
+				// Close the data fork and the resource/info fork writers before
+				// renaming. Closing the fork writers is required for backends
+				// that only commit on Close and avoids leaking descriptors.
 				if err := incWriter.Close(); err != nil {
 					return fmt.Errorf("close file: %v", err)
 				}
+				for _, c := range forkClosers {
+					if err := c.Close(); err != nil {
+						return fmt.Errorf("close fork: %v", err)
+					}
+				}
 
 				// Rename the temporary upload file to the final file name.
 				if err := fileStore.Rename(filePath+".incomplete", filePath); err != nil {
diff --git a/hotline/file_transfer_handlers_test.go b/hotline/file_transfer_handlers_test.go
index 2af95cc..06bc041 100644
--- a/hotline/file_transfer_handlers_test.go
+++ b/hotline/file_transfer_handlers_test.go
@@ -154,6 +154,33 @@ func TestUploadHandler(t *testing.T) {
 		assert.True(t, os.IsNotExist(err))
 	})
 
+	t.Run("commits fork data on a backend that only persists on Close", func(t *testing.T) {
+		// MemFileStore's writers buffer and only commit on Close. This guards
+		// against regressing the fork writers being left unclosed, which would
+		// silently drop the resource and info forks on such backends.
+		fileStore := NewMemFileStore()
+		dst := "/files/note.txt"
+
+		data := []byte("body")
+		rsrc := []byte("rsrc")
+
+		ft := &FileTransfer{bytesSentCounter: &WriteCounter{}}
+		rwc := readWriter{r: bytes.NewReader(flatFileBytes("note.txt", data, rsrc))}
+
+		require.NoError(t, UploadHandler(rwc, dst, ft, fileStore, logger, true))
+
+		got, err := fileStore.ReadFile(dst)
+		require.NoError(t, err)
+		assert.Equal(t, data, got)
+
+		gotRsrc, err := fileStore.ReadFile("/files/" + fmt.Sprintf(RsrcForkNameTemplate, "note.txt"))
+		require.NoError(t, err, "resource fork must be committed")
+		assert.Equal(t, rsrc, gotRsrc)
+
+		_, err = fileStore.ReadFile("/files/" + fmt.Sprintf(InfoForkNameTemplate, "note.txt"))
+		require.NoError(t, err, "info fork must be committed")
+	})
+
 	t.Run("refuses to overwrite an existing file", func(t *testing.T) {
 		dir := t.TempDir()
 		fileStore := &OSFileStore{}