fix: persist media and virtual-device changes across a power cut (#987)

* fix(storage): make entry changes on the data partition survive a power cut

Deleting an image only touched the page cache: the unlink was not followed
by an fsync of the directory, and the data partition is exFAT, which has no
journal. A power cut before writeback rolled the directory entry back, so
the image the user deleted was back after the reboot - it kept its original
inode and timestamps, which is what a rolled back dirent looks like.

The same gap applied to the rename that installs an uploaded or downloaded
image and to mkdir: the name pointing at the entries was never synced. On
top of that both transfer paths closed their temporary file without an
fsync, so even a correct rename could publish an image whose bytes were
still only in the page cache.

Sync the directory after every entry change, sync the transferred image
before the rename that publishes it, and do the two transfer flushes outside
the media lock: on exFAT an inode fsync also writes the device's dirty
metadata and flushes its cache, and an image is multi-gigabyte.

A sync failure on the delete paths is logged rather than returned: the entry
is already gone from the running system, so the caller can only be told that
it may come back.

* fix(vm): make the virtual device toggle survive a power cut

The virtual USB disk and the virtual network are switched on and off by a
marker file on /boot that the boot script reads. The toggle created or
removed that marker and restarted the gadget, but never synced /boot: it is
FAT, without a journal, so the entry stayed in the page cache and a power cut
right after the switch brought the marker back. A device the user turned off
was on again after the reboot, and one turned on could come up off.

Sync the /boot directory once the marker commands ran.

server/service/download/service.go

@@ -390,10 +390,18 @@ func (s *Service) DownloadImageFile(c *gin.Context) {
 		// fails when it is mounted, not while it is stored.
 		_, copyErr := io.Copy(lw, part)
 		lw.Stop()
+		// The bytes have to be on the medium before the rename publishes the
+		// name, and the sync happens here rather than under the media lock
+		// the install takes: an image is large and the flush is slow. A
+		// failed copy has nothing worth syncing, it is removed below.
+		var syncErr error
+		if copyErr == nil {
+			syncErr = out.Sync()
+		}
 		partCloseErr := part.Close()
 		outCloseErr := out.Close()
-		if copyErr != nil || partCloseErr != nil || outCloseErr != nil {
-			log.Errorf("write uploaded image %q failed: %s", filename, errors.Join(copyErr, partCloseErr, outCloseErr))
+		if copyErr != nil || syncErr != nil || partCloseErr != nil || outCloseErr != nil {
+			log.Errorf("write uploaded image %q failed: %s", filename, errors.Join(copyErr, syncErr, partCloseErr, outCloseErr))
 			rsp.ErrRsp(c, -1, "write failed")
 			return
 		}
@@ -572,6 +580,14 @@ func (s *Service) downloadRemoteImage(
 	lw := utils.NewProgressWriter(io.MultiWriter(tempFile, hasher), resp.ContentLength, onProgress)
 	written, copyErr := io.Copy(lw, io.LimitReader(resp.Body, limit+1))
 	lw.Stop()
+	// Same as the upload side: the image is not installable until its bytes
+	// are on the medium, and the flush is done outside the media lock. A
+	// cancelled or failed download is removed by the caller, so it is not
+	// worth flushing it first.
+	var syncErr error
+	if copyErr == nil && ctx.Err() == nil {
+		syncErr = tempFile.Sync()
+	}
 	closeErr := tempFile.Close()
 	if ctx.Err() != nil {
 		return ctx.Err()
@@ -579,6 +595,9 @@ func (s *Service) downloadRemoteImage(
 	if copyErr != nil {
 		return fmt.Errorf("save downloaded image failed: %w", copyErr)
 	}
+	if syncErr != nil {
+		return fmt.Errorf("sync downloaded image failed: %w", syncErr)
+	}
 	if closeErr != nil {
 		return fmt.Errorf("close downloaded image failed: %w", closeErr)
 	}

server/service/storage/diskfs/diskfs_test.go

@@ -179,3 +179,17 @@ func TestMkdirDir(t *testing.T) {
 		t.Error("mkdirDir created a directory outside the medium")
 	}
 }
+
+// SyncDir is what makes a delete or a rename durable on the exFAT data
+// partition; it has to work on a plain directory and report a directory that
+// is not there instead of pretending the entry was flushed.
+func TestSyncDir(t *testing.T) {
+	dir := t.TempDir()
+
+	if err := SyncDir(dir); err != nil {
+		t.Fatalf("SyncDir(%q) = %v, want nil", dir, err)
+	}
+	if err := SyncDir(filepath.Join(dir, "missing")); err == nil {
+		t.Error("SyncDir on a missing directory returned nil")
+	}
+}

server/service/storage/diskfs/mount.go

@@ -474,6 +474,11 @@ func writeFile(root, rel string, r io.Reader) (int64, error) {
 		_ = os.Remove(tempPath)
 		return 0, err
 	}
+	// The file itself is synced above; the name that now points at it is not
+	// until the directory is written back.
+	if err := SyncDir(filepath.Dir(full)); err != nil {
+		logrus.Warnf("upload %s: %s", rel, err)
+	}
 
 	return written, nil
 }
@@ -511,6 +516,36 @@ func mkdirDir(root, rel string) error {
 	if err := os.Mkdir(full, 0o755); err != nil {
 		return fmt.Errorf("create directory %s: %w", rel, err)
 	}
+	// The directory is empty but it is not durable until the entry in its
+	// parent is written back, same as a delete.
+	if err := SyncDir(filepath.Dir(full)); err != nil {
+		logrus.Warnf("create directory %s: %s", rel, err)
+	}
+	return nil
+}
+
+// SyncDir commits the directory entry changes made in dir: a creation, a
+// rename or an unlink is only in the page cache until the directory itself is
+// written back. The data partition is exFAT, which has no journal, so a power
+// cut before that writeback simply rolls the entry back: a file the user
+// deleted is there again after the reboot, and an image that finished
+// uploading is missing its last entries. fsync() on the directory is what
+// makes those changes survive.
+//
+// It is not a cheap directory-only write on exFAT: the inode's fsync also
+// writes the device's dirty metadata buffers and flushes the device cache, so
+// one call can cost a card flush. Callers that change many entries at once
+// should sync once at the end instead of per entry.
+func SyncDir(dir string) error {
+	handle, err := os.Open(dir)
+	if err != nil {
+		return fmt.Errorf("open %s: %w", dir, err)
+	}
+	defer handle.Close()
+
+	if err := handle.Sync(); err != nil {
+		return fmt.Errorf("sync %s: %w", dir, err)
+	}
 	return nil
 }
 
@@ -546,7 +581,17 @@ func removeFile(root, rel string) error {
 	// A directory goes with everything it holds: the web UI confirms the
 	// delete first, and answering an explicit yes with "directory not empty"
 	// helps no one. ResolveInside keeps the walk inside the medium.
-	return os.RemoveAll(full)
+	if err := os.RemoveAll(full); err != nil {
+		return err
+	}
+	// Without this the unlink stays in the page cache, and on the exFAT data
+	// partition a power cut before the writeback brings the file back. A sync
+	// failure does not turn a done delete into an error: the entry is already
+	// gone from the running system, the failure only says it may return.
+	if err := SyncDir(filepath.Dir(full)); err != nil {
+		logrus.Warnf("delete %s: %s", rel, err)
+	}
+	return nil
 }
 
 func (h *handle) Close() error {

server/service/storage/media.go

@@ -226,6 +226,14 @@ func deleteMedium(backing string) error {
 	if err := os.Remove(backing); err != nil {
 		return fmt.Errorf("remove %s: %w", backing, err)
 	}
+	// The name is only gone from the page cache until the directory is written
+	// back, and the data partition is exFAT: without the sync a power cut
+	// before writeback brings the deleted image back on the next boot. Images
+	// live in subdirectories too, so the entry to sync is the one holding
+	// this file, not the media root.
+	if err := diskfs.SyncDir(filepath.Dir(backing)); err != nil {
+		log.Warnf("delete %s: %s", backing, err)
+	}
 	return nil
 }
 
@@ -597,6 +605,10 @@ func openMediaIDs() []string {
 // image. The same lock used by gadget and file-manager operations covers both
 // the checks and the rename, so the destination cannot become attached or
 // loop-mounted in between.
+//
+// The caller owns tempPath and must have synced its bytes: this only makes the
+// name durable, the flush of a multi-gigabyte image does not belong under the
+// media lock.
 func InstallImage(tempPath, destPath string) error {
 	mediumOperationMu.Lock()
 	defer mediumOperationMu.Unlock()
@@ -628,6 +640,13 @@ func installImage(tempPath, destPath string, status diskStatus, mounted bool) er
 	if err := os.Rename(tempPath, destPath); err != nil {
 		return fmt.Errorf("install image %s: %w", filepath.Base(destPath), err)
 	}
+	// The caller synced the bytes of tempPath, the name is what is still only
+	// in the page cache. Logged like the other sync failures: the image is on
+	// the medium now, but a power cut can take it away, and nothing else would
+	// ever say so.
+	if err := diskfs.SyncDir(filepath.Dir(destPath)); err != nil {
+		log.Warnf("install image %s: %s", destPath, err)
+	}
 	return nil
 }
 

server/service/vm/virtual-device.go

@@ -11,11 +11,13 @@ import (
 	"NanoKVM-Server/proto"
 	"NanoKVM-Server/service/hid"
 	"NanoKVM-Server/service/storage"
+	"NanoKVM-Server/service/storage/diskfs"
 )
 
 const (
 	virtualNetwork = "/boot/usb.rndis0"
 	virtualDisk    = "/boot/usb.disk0"
+	bootMountPoint = "/boot"
 )
 
 var (
@@ -173,6 +175,17 @@ func (s *Service) UpdateVirtualDevice(c *gin.Context) {
 		}
 	}
 
+	// The marker that turns a virtual device on or off lives on /boot, a FAT
+	// partition without a journal: the create or the unlink is only in the
+	// page cache until the directory is written back. A power cut right after
+	// the toggle brings the old marker back, so a device the user switched off
+	// is on again on the next boot.
+	if len(commands) > 0 {
+		if err := diskfs.SyncDir(bootMountPoint); err != nil {
+			log.Warnf("sync %s after virtual device change: %s", bootMountPoint, err)
+		}
+	}
+
 	// Whatever the scripts got to, /data belongs to this side again unless
 	// the raw partition really is served to the target machine now. That
 	// covers a failed script, which may have stopped before re-attaching the