Tolerate a still-locked updater binary when cleaning up after an update

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:

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)
+	}
+}

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) {
+	if err := removeUpdaterBinary(filepath.Join(u.tempDir, updaterBinary)); err != nil {
-		merr = multierror.Append(merr, fmt.Errorf("failed to remove updater binary: %w", err))
+		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
 			}

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)
+	}
+}

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
+}

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
+}

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)
+	}
+}