From 24d1e4f9b99dd1aa30f8df345ec347cb43c2d88d Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Fri, 21 Aug 2026 15:40:01 +0200 Subject: [PATCH] Tolerate a still-locked updater binary when cleaning up after an update --- client/internal/updater/installer/doc.go | 4 ++ .../installer_cleanup_windows_test.go | 67 +++++++++++++++++++ .../updater/installer/installer_common.go | 12 +++- .../installer/installer_common_test.go | 52 ++++++++++++++ .../installer/remove_updater_darwin.go | 12 ++++ .../installer/remove_updater_windows.go | 58 ++++++++++++++++ .../installer/remove_updater_windows_test.go | 32 +++++++++ 7 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 client/internal/updater/installer/installer_cleanup_windows_test.go create mode 100644 client/internal/updater/installer/installer_common_test.go create mode 100644 client/internal/updater/installer/remove_updater_darwin.go create mode 100644 client/internal/updater/installer/remove_updater_windows.go create mode 100644 client/internal/updater/installer/remove_updater_windows_test.go diff --git a/client/internal/updater/installer/doc.go b/client/internal/updater/installer/doc.go index 11b0512ac..4d624cbb6 100644 --- a/client/internal/updater/installer/doc.go +++ b/client/internal/updater/installer/doc.go @@ -109,6 +109,10 @@ // - Does NOT remove result.json (cleaned by ResultHandler after read) // - Does NOT remove msi.log (kept for debugging) // +// On Windows the updater copy is often still locked when the daemon it restarted +// runs cleanup, so removing it is retried briefly and otherwise deferred to the +// next reboot rather than reported as a failure. +// // # Dry-Run Mode // // Dry-run mode allows testing the update process without actually installing: diff --git a/client/internal/updater/installer/installer_cleanup_windows_test.go b/client/internal/updater/installer/installer_cleanup_windows_test.go new file mode 100644 index 000000000..aab16dc93 --- /dev/null +++ b/client/internal/updater/installer/installer_cleanup_windows_test.go @@ -0,0 +1,67 @@ +package installer + +import ( + "os" + "path/filepath" + "testing" + "time" + + "golang.org/x/sys/windows" +) + +// lockFile opens path without FILE_SHARE_DELETE, so os.Remove fails the way it does +// while the updater process still holds its own image. +func lockFile(t *testing.T, path string) windows.Handle { + t.Helper() + + p, err := windows.UTF16PtrFromString(path) + if err != nil { + t.Fatalf("convert path: %v", err) + } + + handle, err := windows.CreateFile(p, windows.GENERIC_READ, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + t.Fatalf("lock %s: %v", path, err) + } + return handle +} + +// releaseAfter closes the handle once the delay has passed, standing in for the +// updater process finally exiting. +func releaseAfter(t *testing.T, handle windows.Handle, delay time.Duration) { + t.Helper() + + released := make(chan struct{}) + t.Cleanup(func() { <-released }) + + go func() { + defer close(released) + time.Sleep(delay) + if err := windows.CloseHandle(handle); err != nil { + t.Errorf("close handle: %v", err) + } + }() +} + +// TestCleanUpInstallerFilesLockedUpdater covers the post-update cleanup race: the +// daemon cleans up at startup while the updater that restarted it is still exiting, +// so the updater image is locked and Windows refuses the delete. Cleanup must wait +// the lock out instead of reporting a failure and leaving the binary behind. +func TestCleanUpInstallerFilesLockedUpdater(t *testing.T) { + tempDir := t.TempDir() + path := filepath.Join(tempDir, updaterBinary) + if err := os.WriteFile(path, []byte("x"), 0o600); err != nil { + t.Fatalf("write updater: %v", err) + } + + releaseAfter(t, lockFile(t, path), 300*time.Millisecond) + + u := NewWithDir(tempDir) + if err := u.CleanUpInstallerFiles(); err != nil { + t.Fatalf("cleanup must tolerate a still-locked updater: %v", err) + } + + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("updater binary still present (stat err: %v)", err) + } +} diff --git a/client/internal/updater/installer/installer_common.go b/client/internal/updater/installer/installer_common.go index 17566f7de..f917424b8 100644 --- a/client/internal/updater/installer/installer_common.go +++ b/client/internal/updater/installer/installer_common.go @@ -152,8 +152,8 @@ func (u *Installer) CleanUpInstallerFiles() error { var merr *multierror.Error - if err := os.Remove(filepath.Join(u.tempDir, updaterBinary)); err != nil && !os.IsNotExist(err) { - merr = multierror.Append(merr, fmt.Errorf("failed to remove updater binary: %w", err)) + if err := removeUpdaterBinary(filepath.Join(u.tempDir, updaterBinary)); err != nil { + merr = multierror.Append(merr, fmt.Errorf("remove updater binary: %w", err)) } entries, err := os.ReadDir(u.tempDir) @@ -167,10 +167,16 @@ func (u *Installer) CleanUpInstallerFiles() error { } name := entry.Name() + // The updater copy is handled above; on Windows its name also matches the + // extension sweep, which would report the same file twice. + if strings.EqualFold(name, updaterBinary) { + continue + } + for _, ext := range binaryExtensions { if strings.HasSuffix(strings.ToLower(name), strings.ToLower(ext)) { if err := os.Remove(filepath.Join(u.tempDir, name)); err != nil { - merr = multierror.Append(merr, fmt.Errorf("failed to remove %s: %w", name, err)) + merr = multierror.Append(merr, fmt.Errorf("remove %s: %w", name, err)) } break } diff --git a/client/internal/updater/installer/installer_common_test.go b/client/internal/updater/installer/installer_common_test.go new file mode 100644 index 000000000..c1556c828 --- /dev/null +++ b/client/internal/updater/installer/installer_common_test.go @@ -0,0 +1,52 @@ +//go:build windows || darwin + +package installer + +import ( + "os" + "path/filepath" + "testing" +) + +// TestCleanUpInstallerFiles checks that cleanup removes the updater copy and the +// downloaded installer while leaving the logs and the result file for the daemon. +func TestCleanUpInstallerFiles(t *testing.T) { + tempDir := t.TempDir() + + installers := make([]string, 0, len(binaryExtensions)) + for _, ext := range binaryExtensions { + installers = append(installers, "netbird_installer."+ext) + } + + kept := []string{"installer.log", "result.json"} + + for _, name := range append(append([]string{updaterBinary}, installers...), kept...) { + if err := os.WriteFile(filepath.Join(tempDir, name), []byte("x"), 0o600); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + + u := NewWithDir(tempDir) + if err := u.CleanUpInstallerFiles(); err != nil { + t.Fatalf("CleanUpInstallerFiles: %v", err) + } + + for _, name := range append([]string{updaterBinary}, installers...) { + if _, err := os.Stat(filepath.Join(tempDir, name)); !os.IsNotExist(err) { + t.Errorf("%s was not removed (stat err: %v)", name, err) + } + } + + for _, name := range kept { + if _, err := os.Stat(filepath.Join(tempDir, name)); err != nil { + t.Errorf("%s should have been kept: %v", name, err) + } + } +} + +func TestCleanUpInstallerFilesMissingTempDir(t *testing.T) { + u := NewWithDir(filepath.Join(t.TempDir(), "does-not-exist")) + if err := u.CleanUpInstallerFiles(); err != nil { + t.Errorf("a missing temp dir is not a cleanup failure, got: %v", err) + } +} diff --git a/client/internal/updater/installer/remove_updater_darwin.go b/client/internal/updater/installer/remove_updater_darwin.go new file mode 100644 index 000000000..4d4a0be60 --- /dev/null +++ b/client/internal/updater/installer/remove_updater_darwin.go @@ -0,0 +1,12 @@ +package installer + +import "os" + +// removeUpdaterBinary deletes the updater copy left in the temp dir. On darwin a +// running binary can be unlinked, so no retry is needed. +func removeUpdaterBinary(path string) error { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return err + } + return nil +} diff --git a/client/internal/updater/installer/remove_updater_windows.go b/client/internal/updater/installer/remove_updater_windows.go new file mode 100644 index 000000000..df120e881 --- /dev/null +++ b/client/internal/updater/installer/remove_updater_windows.go @@ -0,0 +1,58 @@ +package installer + +import ( + "errors" + "fmt" + "os" + "time" + + log "github.com/sirupsen/logrus" + "golang.org/x/sys/windows" +) + +const ( + // The updater is the process that restarted the daemon, so when the daemon + // cleans up at startup the updater is often still exiting and Windows refuses + // to delete its locked image. These bound how long cleanup waits for it. + updaterRemoveAttempts = 5 + updaterRemoveDelay = 200 * time.Millisecond +) + +// removeUpdaterBinary deletes the updater copy left in the temp dir, retrying +// while the still-exiting updater process holds its image. If it stays locked for +// the whole window the file is scheduled for deletion on the next reboot, so a +// locked updater is never reported as a cleanup failure. +func removeUpdaterBinary(path string) error { + for attempt := 0; attempt < updaterRemoveAttempts; attempt++ { + if attempt > 0 { + time.Sleep(updaterRemoveDelay) + } + + err := os.Remove(path) + if err == nil || os.IsNotExist(err) { + return nil + } + if !isFileLocked(err) { + return err + } + } + + log.Debugf("updater binary %s is still locked, scheduling removal on next reboot", path) + return scheduleDeleteOnReboot(path) +} + +func isFileLocked(err error) bool { + return errors.Is(err, windows.ERROR_ACCESS_DENIED) || errors.Is(err, windows.ERROR_SHARING_VIOLATION) +} + +func scheduleDeleteOnReboot(path string) error { + from, err := windows.UTF16PtrFromString(path) + if err != nil { + return fmt.Errorf("convert path to UTF16: %w", err) + } + + if err := windows.MoveFileEx(from, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT); err != nil { + return fmt.Errorf("schedule delete on reboot: %w", err) + } + return nil +} diff --git a/client/internal/updater/installer/remove_updater_windows_test.go b/client/internal/updater/installer/remove_updater_windows_test.go new file mode 100644 index 000000000..195d1d8ad --- /dev/null +++ b/client/internal/updater/installer/remove_updater_windows_test.go @@ -0,0 +1,32 @@ +package installer + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestRemoveUpdaterBinaryRetriesWhileLocked(t *testing.T) { + path := filepath.Join(t.TempDir(), updaterBinary) + if err := os.WriteFile(path, []byte("x"), 0o600); err != nil { + t.Fatalf("write updater: %v", err) + } + + releaseAfter(t, lockFile(t, path), updaterRemoveDelay+50*time.Millisecond) + + if err := removeUpdaterBinary(path); err != nil { + t.Fatalf("removeUpdaterBinary: %v", err) + } + + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("updater binary still present (stat err: %v)", err) + } +} + +func TestRemoveUpdaterBinaryMissingFile(t *testing.T) { + path := filepath.Join(t.TempDir(), updaterBinary) + if err := removeUpdaterBinary(path); err != nil { + t.Errorf("a missing updater binary is not a failure, got: %v", err) + } +}