From 505049a4de2f2411b81910de6a7345df3552e00b Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 4 Aug 2026 21:02:52 -0700 Subject: [PATCH] volume: skip directory fsync on Windows, report a failed makeupDiff (#10572) * volume: skip directory fsync on Windows * ci: run the windows jobs for the whole vacuum path Both windows jobs start the same weed mini cluster, so both exercise the volume server's vacuum path, but only one of them watched a single file in it. Cover the compact, reconcile and load files in both. * volume: report a failed makeupDiff instead of discarding it The cleanup removes assigned to the same err the makeupDiff failure was held in, so an aborted compaction returned nil once both removes succeeded. The master then recorded the vacuum as committed and the volume reloaded against the discarded generation. * volume: correct the fsyncDir comments after the windows skip Both comments described the old shape, where windows fell through to a sync whose error was swallowed. * volume: keep the makeupDiff failure ahead of its cleanup errors A failed remove of .cpd/.cpx outranked the failure that abandoned the compaction, so the caller saw the cleanup error instead of the cause. Log it and return the original, matching the Rust do_commit_compact. A leftover temp file is rolled back by reconcile on the next start. --- .../workflows/mount-windows-conformance.yml | 6 +++ .github/workflows/mount-windows.yml | 6 +++ seaweed-volume/src/storage/volume.rs | 2 +- weed/storage/volume_vacuum.go | 52 ++++++++++--------- 4 files changed, 41 insertions(+), 25 deletions(-) diff --git a/.github/workflows/mount-windows-conformance.yml b/.github/workflows/mount-windows-conformance.yml index 95b0ea4b9..6d490c5d8 100644 --- a/.github/workflows/mount-windows-conformance.yml +++ b/.github/workflows/mount-windows-conformance.yml @@ -6,6 +6,9 @@ on: paths: - 'weed/mount/**' - 'weed/command/mount*.go' + - 'weed/storage/volume_vacuum*.go' + - 'weed/storage/volume_loading.go' + - 'weed/storage/disk_location.go' - 'test/winfsp-conformance/**' - '.github/workflows/mount-windows-conformance.yml' # No base branch filter: this is the only thing that runs the Windows mount, @@ -14,6 +17,9 @@ on: paths: - 'weed/mount/**' - 'weed/command/mount*.go' + - 'weed/storage/volume_vacuum*.go' + - 'weed/storage/volume_loading.go' + - 'weed/storage/disk_location.go' - 'test/winfsp-conformance/**' - '.github/workflows/mount-windows-conformance.yml' diff --git a/.github/workflows/mount-windows.yml b/.github/workflows/mount-windows.yml index 985f8c85c..cd9db5685 100644 --- a/.github/workflows/mount-windows.yml +++ b/.github/workflows/mount-windows.yml @@ -6,6 +6,9 @@ on: paths: - 'weed/mount/**' - 'weed/command/mount*.go' + - 'weed/storage/volume_vacuum*.go' + - 'weed/storage/volume_loading.go' + - 'weed/storage/disk_location.go' - 'test/winfsp/**' - '.github/workflows/mount-windows.yml' # No base branch filter: this is the only thing that runs the Windows mount, @@ -14,6 +17,9 @@ on: paths: - 'weed/mount/**' - 'weed/command/mount*.go' + - 'weed/storage/volume_vacuum*.go' + - 'weed/storage/volume_loading.go' + - 'weed/storage/disk_location.go' - 'test/winfsp/**' - '.github/workflows/mount-windows.yml' diff --git a/seaweed-volume/src/storage/volume.rs b/seaweed-volume/src/storage/volume.rs index 3c00f1866..01896350e 100644 --- a/seaweed-volume/src/storage/volume.rs +++ b/seaweed-volume/src/storage/volume.rs @@ -3627,7 +3627,7 @@ fn get_append_at_ns(last: u64) -> u64 { /// durable, propagating a sync failure so the commit path can abort rather than /// proceed with an undurable rename or marker. A path with no openable parent is /// tolerated; directory fsync is unsupported on Windows, so it is a no-op there -/// (matching the Go fsyncDir helper, which ignores that error). +/// (matching the Go fsyncDir helper, which skips it too). pub(crate) fn fsync_dir(path: &str) -> io::Result<()> { #[cfg(windows)] { diff --git a/weed/storage/volume_vacuum.go b/weed/storage/volume_vacuum.go index 94ae586a9..a1e2835ed 100644 --- a/weed/storage/volume_vacuum.go +++ b/weed/storage/volume_vacuum.go @@ -187,36 +187,36 @@ func (v *Volume) CommitCompact() error { v.DataBackend = nil stats.VolumeServerVolumeGauge.WithLabelValues(v.Collection, "volume").Dec() - var e error - if e = v.makeupDiff(v.FileName(".cpd"), v.FileName(".cpx"), v.FileName(".dat"), v.FileName(".idx")); e != nil { - glog.V(0).Infof("makeupDiff in CommitCompact volume %d failed %v", v.Id, e) - e = os.Remove(v.FileName(".cpd")) - if e != nil { - return e + if compactErr := v.makeupDiff(v.FileName(".cpd"), v.FileName(".cpx"), v.FileName(".dat"), v.FileName(".idx")); compactErr != nil { + glog.V(0).Infof("makeupDiff in CommitCompact volume %d failed %v", v.Id, compactErr) + if e := os.Remove(v.FileName(".cpd")); e != nil && !os.IsNotExist(e) { + glog.V(0).Infof("remove %s: %v", v.FileName(".cpd"), e) } - e = os.Remove(v.FileName(".cpx")) - if e != nil { - return e - } - } else { - // makeupDiff has fsynced the .cpd/.cpx contents. Persist a durable .cpc - // commit marker BEFORE renaming so the two renames are atomic across a - // crash: a marker on disk means the swap is decided and reconcile rolls - // forward; no marker means roll back. Without it, a crash between the - // two renames leaves a stale .idx that a later vacuum compacts to empty. - if e = v.writeCompactCommitMarker(); e != nil { - return e - } - if e = v.applyCompactSwap(); e != nil { - return e + if e := os.Remove(v.FileName(".cpx")); e != nil && !os.IsNotExist(e) { + glog.V(0).Infof("remove %s: %v", v.FileName(".cpx"), e) } + // Report the abandoned compaction rather than a cleanup failure that + // reconcile rolls back anyway, and never fall through to the reload. + return compactErr + } + + // makeupDiff has fsynced the .cpd/.cpx contents. Persist a durable .cpc + // commit marker BEFORE renaming so the two renames are atomic across a + // crash: a marker on disk means the swap is decided and reconcile rolls + // forward; no marker means roll back. Without it, a crash between the + // two renames leaves a stale .idx that a later vacuum compacts to empty. + if e := v.writeCompactCommitMarker(); e != nil { + return e + } + if e := v.applyCompactSwap(); e != nil { + return e } //glog.V(3).Infof("Pretending to be vacuuming...") //time.Sleep(20 * time.Second) glog.V(3).Infof("Loading volume %d commit file...", v.Id) - if e = v.load(true, false, v.needleMapKind, 0, v.Version()); e != nil { + if e := v.load(true, false, v.needleMapKind, 0, v.Version()); e != nil { return e } glog.V(3).Infof("Finish committing volume %d", v.Id) @@ -377,9 +377,13 @@ func (v *Volume) cleanupCompact() error { } // fsyncDir fsyncs a directory so a rename/create/unlink inside it is durable. -// A failure to open the directory for sync is non-fatal on platforms that do -// not support it. +// Windows has no directory fsync, so the .cpc protocol leans on NTFS metadata +// ordering there. An unopenable directory is tolerated, unlike util.FsyncDir, +// because the caller has already closed the needle map. func fsyncDir(dir string) error { + if runtime.GOOS == "windows" { + return nil + } d, err := os.Open(dir) if err != nil { return nil