From ba5c89fe6690e4c795acd61b38e267b952bdf897 Mon Sep 17 00:00:00 2001 From: thazjswe42700 <131556390+thazjswe42700@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:48:19 +0800 Subject: [PATCH 1/2] fix: make atomic state writes work on Windows The directory fsync issued after rename in backup and backuptransfer is now best-effort. Windows rejects FlushFileBuffers on a read-only directory handle with ERROR_ACCESS_DENIED, so the server aborted at startup with "write server identity: ... Access is denied" even though the rename had already committed and the file on disk was intact. This mirrors the convention already documented in internal/config. Also skip the Unix permission guard on backup transfer state files when running on Windows: Chmod only toggles the read-only attribute there and Lstat always reports 0666 for a writable file, so the guard rejected every state file the process had just written itself. Access is governed by the directory ACL on that platform. Co-Authored-By: Claude Opus 5 (1M context) --- backend/internal/backup/manager.go | 18 +++++++----------- backend/internal/backuptransfer/store.go | 24 ++++++++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/backend/internal/backup/manager.go b/backend/internal/backup/manager.go index 0540e7e3..a000f015 100644 --- a/backend/internal/backup/manager.go +++ b/backend/internal/backup/manager.go @@ -1124,17 +1124,13 @@ func writeJSONAtomic(filePath string, value any, mode os.FileMode) error { return err } removeTemporary = false - directoryHandle, err := os.Open(directory) - if err != nil { - return err - } - syncErr := directoryHandle.Sync() - closeErr := directoryHandle.Close() - if syncErr != nil { - return syncErr - } - if closeErr != nil { - return closeErr + // Directory Sync is best-effort because several supported filesystems (and + // Windows) reject syncing directory handles. The rename has already committed + // at this point, so returning an error would falsely tell callers that the + // write failed even though the file was replaced. + if directoryHandle, err := os.Open(directory); err == nil { + _ = directoryHandle.Sync() + _ = directoryHandle.Close() } return nil } diff --git a/backend/internal/backuptransfer/store.go b/backend/internal/backuptransfer/store.go index 454ff878..35c6c233 100644 --- a/backend/internal/backuptransfer/store.go +++ b/backend/internal/backuptransfer/store.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "runtime" "strings" "time" @@ -59,7 +60,11 @@ func readJSONFile(path string, destination any) error { if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { return errors.New("backup transfer: state is not a regular file") } - if info.Mode().Perm()&0o077 != 0 { + // Windows has no Unix permission bits: Chmod only toggles the read-only + // attribute and Lstat always reports 0666 for a writable file, so this guard + // would reject every state file we just wrote ourselves. Access there is + // governed by the directory ACL instead. + if runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { return errors.New("backup transfer: state file permissions are too broad") } if info.Size() > maxStateFileBytes { @@ -119,16 +124,15 @@ func writeJSONAtomic(path string, value any) error { return err } removeTemporary = false - directoryHandle, err := os.Open(directory) - if err != nil { - return err + // Directory Sync is best-effort because several supported filesystems (and + // Windows) reject syncing directory handles. The rename has already committed + // at this point, so returning an error would falsely tell callers that the + // state write failed even though the file was replaced. + if directoryHandle, err := os.Open(directory); err == nil { + _ = directoryHandle.Sync() + _ = directoryHandle.Close() } - syncErr := directoryHandle.Sync() - closeErr := directoryHandle.Close() - if syncErr != nil { - return syncErr - } - return closeErr + return nil } func validOpaqueID(value string) bool { From 18dced9226b518b67b34d69e96d10dfd25aa3a5a Mon Sep 17 00:00:00 2001 From: nianzhibai Date: Fri, 14 Aug 2026 14:29:02 +0000 Subject: [PATCH 2/2] fix: preserve durable state writes off Windows --- backend/internal/atomicfile/syncdir.go | 10 ++++++++ .../internal/atomicfile/syncdir_nonwindows.go | 18 +++++++++++++++ .../atomicfile/syncdir_nonwindows_test.go | 16 +++++++++++++ backend/internal/atomicfile/syncdir_test.go | 9 ++++++++ .../internal/atomicfile/syncdir_windows.go | 10 ++++++++ backend/internal/backup/manager.go | 11 ++------- .../backuptransfer/permissions_nonwindows.go | 9 ++++++++ .../permissions_nonwindows_test.go | 23 +++++++++++++++++++ .../backuptransfer/permissions_windows.go | 12 ++++++++++ .../permissions_windows_test.go | 14 +++++++++++ backend/internal/backuptransfer/store.go | 18 +++------------ backend/internal/config/config.go | 6 ++--- 12 files changed, 128 insertions(+), 28 deletions(-) create mode 100644 backend/internal/atomicfile/syncdir.go create mode 100644 backend/internal/atomicfile/syncdir_nonwindows.go create mode 100644 backend/internal/atomicfile/syncdir_nonwindows_test.go create mode 100644 backend/internal/atomicfile/syncdir_test.go create mode 100644 backend/internal/atomicfile/syncdir_windows.go create mode 100644 backend/internal/backuptransfer/permissions_nonwindows.go create mode 100644 backend/internal/backuptransfer/permissions_nonwindows_test.go create mode 100644 backend/internal/backuptransfer/permissions_windows.go create mode 100644 backend/internal/backuptransfer/permissions_windows_test.go diff --git a/backend/internal/atomicfile/syncdir.go b/backend/internal/atomicfile/syncdir.go new file mode 100644 index 00000000..f4a2cf78 --- /dev/null +++ b/backend/internal/atomicfile/syncdir.go @@ -0,0 +1,10 @@ +// Package atomicfile contains the platform-specific durability steps used by +// same-directory atomic file replacements. +package atomicfile + +// SyncDirectory makes a completed rename durable on platforms that support +// syncing directory handles. Platforms without that operation may implement +// this as a no-op after the renamed file itself has been synced. +func SyncDirectory(directory string) error { + return syncDirectory(directory) +} diff --git a/backend/internal/atomicfile/syncdir_nonwindows.go b/backend/internal/atomicfile/syncdir_nonwindows.go new file mode 100644 index 00000000..3f8e788c --- /dev/null +++ b/backend/internal/atomicfile/syncdir_nonwindows.go @@ -0,0 +1,18 @@ +//go:build !windows + +package atomicfile + +import "os" + +func syncDirectory(directory string) error { + directoryHandle, err := os.Open(directory) + if err != nil { + return err + } + syncErr := directoryHandle.Sync() + closeErr := directoryHandle.Close() + if syncErr != nil { + return syncErr + } + return closeErr +} diff --git a/backend/internal/atomicfile/syncdir_nonwindows_test.go b/backend/internal/atomicfile/syncdir_nonwindows_test.go new file mode 100644 index 00000000..7fa7ae78 --- /dev/null +++ b/backend/internal/atomicfile/syncdir_nonwindows_test.go @@ -0,0 +1,16 @@ +//go:build !windows + +package atomicfile + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSyncDirectoryReturnsOpenErrors(t *testing.T) { + missing := filepath.Join(t.TempDir(), "missing") + if err := SyncDirectory(missing); !os.IsNotExist(err) { + t.Fatalf("SyncDirectory error = %v, want not-exist error", err) + } +} diff --git a/backend/internal/atomicfile/syncdir_test.go b/backend/internal/atomicfile/syncdir_test.go new file mode 100644 index 00000000..fce1f726 --- /dev/null +++ b/backend/internal/atomicfile/syncdir_test.go @@ -0,0 +1,9 @@ +package atomicfile + +import "testing" + +func TestSyncDirectoryAcceptsExistingDirectory(t *testing.T) { + if err := SyncDirectory(t.TempDir()); err != nil { + t.Fatal(err) + } +} diff --git a/backend/internal/atomicfile/syncdir_windows.go b/backend/internal/atomicfile/syncdir_windows.go new file mode 100644 index 00000000..ce3fc078 --- /dev/null +++ b/backend/internal/atomicfile/syncdir_windows.go @@ -0,0 +1,10 @@ +//go:build windows + +package atomicfile + +func syncDirectory(string) error { + // os.File.Sync uses FlushFileBuffers on Windows. Directory handles opened + // through os.Open cannot be flushed and return ERROR_ACCESS_DENIED, even + // though the preceding rename has already completed. + return nil +} diff --git a/backend/internal/backup/manager.go b/backend/internal/backup/manager.go index a000f015..a91b31de 100644 --- a/backend/internal/backup/manager.go +++ b/backend/internal/backup/manager.go @@ -18,6 +18,7 @@ import ( "sync" "time" + "github.com/video-site/backend/internal/atomicfile" "github.com/video-site/backend/internal/catalog" "github.com/video-site/backend/internal/config" "github.com/video-site/backend/internal/localpath" @@ -1124,15 +1125,7 @@ func writeJSONAtomic(filePath string, value any, mode os.FileMode) error { return err } removeTemporary = false - // Directory Sync is best-effort because several supported filesystems (and - // Windows) reject syncing directory handles. The rename has already committed - // at this point, so returning an error would falsely tell callers that the - // write failed even though the file was replaced. - if directoryHandle, err := os.Open(directory); err == nil { - _ = directoryHandle.Sync() - _ = directoryHandle.Close() - } - return nil + return atomicfile.SyncDirectory(directory) } func metaPath(archivePath string) string { diff --git a/backend/internal/backuptransfer/permissions_nonwindows.go b/backend/internal/backuptransfer/permissions_nonwindows.go new file mode 100644 index 00000000..6e02ccf1 --- /dev/null +++ b/backend/internal/backuptransfer/permissions_nonwindows.go @@ -0,0 +1,9 @@ +//go:build !windows + +package backuptransfer + +import "os" + +func stateFilePermissionsTooBroad(mode os.FileMode) bool { + return mode.Perm()&0o077 != 0 +} diff --git a/backend/internal/backuptransfer/permissions_nonwindows_test.go b/backend/internal/backuptransfer/permissions_nonwindows_test.go new file mode 100644 index 00000000..f2ead1ba --- /dev/null +++ b/backend/internal/backuptransfer/permissions_nonwindows_test.go @@ -0,0 +1,23 @@ +//go:build !windows + +package backuptransfer + +import ( + "os" + "testing" +) + +func TestStateFilePermissionsTooBroad(t *testing.T) { + for _, test := range []struct { + mode uint32 + broad bool + }{ + {mode: 0o600, broad: false}, + {mode: 0o640, broad: true}, + {mode: 0o606, broad: true}, + } { + if broad := stateFilePermissionsTooBroad(os.FileMode(test.mode)); broad != test.broad { + t.Fatalf("mode %#o broad = %t, want %t", test.mode, broad, test.broad) + } + } +} diff --git a/backend/internal/backuptransfer/permissions_windows.go b/backend/internal/backuptransfer/permissions_windows.go new file mode 100644 index 00000000..b0cc6dda --- /dev/null +++ b/backend/internal/backuptransfer/permissions_windows.go @@ -0,0 +1,12 @@ +//go:build windows + +package backuptransfer + +import "os" + +func stateFilePermissionsTooBroad(os.FileMode) bool { + // Windows FileMode permission bits are synthetic: writable files report + // 0666, and Chmod only changes the read-only attribute. The state directory + // ACL is the access-control boundary on this platform. + return false +} diff --git a/backend/internal/backuptransfer/permissions_windows_test.go b/backend/internal/backuptransfer/permissions_windows_test.go new file mode 100644 index 00000000..73d46671 --- /dev/null +++ b/backend/internal/backuptransfer/permissions_windows_test.go @@ -0,0 +1,14 @@ +//go:build windows + +package backuptransfer + +import ( + "os" + "testing" +) + +func TestStateFilePermissionsIgnoreSyntheticWindowsBits(t *testing.T) { + if stateFilePermissionsTooBroad(os.FileMode(0o666)) { + t.Fatal("synthetic Windows permission bits were rejected") + } +} diff --git a/backend/internal/backuptransfer/store.go b/backend/internal/backuptransfer/store.go index 35c6c233..095f50f1 100644 --- a/backend/internal/backuptransfer/store.go +++ b/backend/internal/backuptransfer/store.go @@ -7,10 +7,10 @@ import ( "io" "os" "path/filepath" - "runtime" "strings" "time" + "github.com/video-site/backend/internal/atomicfile" "github.com/video-site/backend/internal/backup" ) @@ -60,11 +60,7 @@ func readJSONFile(path string, destination any) error { if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { return errors.New("backup transfer: state is not a regular file") } - // Windows has no Unix permission bits: Chmod only toggles the read-only - // attribute and Lstat always reports 0666 for a writable file, so this guard - // would reject every state file we just wrote ourselves. Access there is - // governed by the directory ACL instead. - if runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { + if stateFilePermissionsTooBroad(info.Mode()) { return errors.New("backup transfer: state file permissions are too broad") } if info.Size() > maxStateFileBytes { @@ -124,15 +120,7 @@ func writeJSONAtomic(path string, value any) error { return err } removeTemporary = false - // Directory Sync is best-effort because several supported filesystems (and - // Windows) reject syncing directory handles. The rename has already committed - // at this point, so returning an error would falsely tell callers that the - // state write failed even though the file was replaced. - if directoryHandle, err := os.Open(directory); err == nil { - _ = directoryHandle.Sync() - _ = directoryHandle.Close() - } - return nil + return atomicfile.SyncDirectory(directory) } func validOpaqueID(value string) bool { diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 29113380..051ee156 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/video-site/backend/internal/atomicfile" "github.com/video-site/backend/internal/localpath" "github.com/video-site/backend/internal/schedule" "gopkg.in/yaml.v3" @@ -493,10 +494,7 @@ func writeFileAtomically(path string, data []byte, mode os.FileMode) error { // Windows) reject syncing directory handles. The rename has already committed // at this point, so returning an error would falsely tell callers that the // save failed even though config.yaml was replaced. - if dirHandle, err := os.Open(dir); err == nil { - _ = dirHandle.Sync() - _ = dirHandle.Close() - } + _ = atomicfile.SyncDirectory(dir) return nil }