From f939a5434c93b78318f99c0ca54314479c8b992b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Fri, 13 Mar 2026 12:56:33 +0100 Subject: [PATCH 01/16] Switch back to os.RemoveAll --- internal/pkg/agent/install/uninstall.go | 108 ++++++++++++------------ 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index c429ce9c258..e8772bb3977 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -285,7 +285,7 @@ func RemovePath(path string) error { start := time.Now() var lastErr error for time.Since(start) <= arbitraryTimeout { - lastErr = removeAll(path) + lastErr = os.RemoveAll(path) if lastErr == nil || !isRetryableError(lastErr) { return lastErr @@ -336,59 +336,59 @@ func RemoveBut(path string, bestEffort bool, exceptions ...string) error { // Taken from: https://cs.opensource.google/go/go/+/refs/tags/go1.24.13:src/os/removeall_noat.go;drc=a2baae6851a157d662dff7cc508659f66249698a;l=15 // The implementation which breaks our install is here: // https://cs.opensource.google/go/go/+/refs/tags/go1.25.8:src/os/removeall_at.go;drc=e81c624656e415626c7ac3a97768f5c2717979a4;l=15 -func removeAll(path string) error { - // Try simple remove first (handles files and empty directories). - err := os.Remove(path) - if err == nil || errors.Is(err, fs.ErrNotExist) { - return nil - } - - // Check if it's a directory. - info, serr := os.Lstat(path) - if serr != nil { - if errors.Is(serr, fs.ErrNotExist) { - return nil - } - return serr - } - if !info.IsDir() { - // Not a directory — return the original Remove error. - return err - } - - // Remove directory contents recursively. - err = removeAllChildren(path) - if err != nil { - return err - } - - // Remove the now-empty directory. - err = os.Remove(path) - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return err -} - -func removeAllChildren(path string) error { - entries, err := os.ReadDir(path) - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return err - } - - var firstErr error - for _, entry := range entries { - child := filepath.Join(path, entry.Name()) - err := removeAll(child) - if err != nil && firstErr == nil { - firstErr = err - } - } - return firstErr -} +//func removeAll(path string) error { +// // Try simple remove first (handles files and empty directories). +// err := os.Remove(path) +// if err == nil || errors.Is(err, fs.ErrNotExist) { +// return nil +// } +// +// // Check if it's a directory. +// info, serr := os.Lstat(path) +// if serr != nil { +// if errors.Is(serr, fs.ErrNotExist) { +// return nil +// } +// return serr +// } +// if !info.IsDir() { +// // Not a directory — return the original Remove error. +// return err +// } +// +// // Remove directory contents recursively. +// err = removeAllChildren(path) +// if err != nil { +// return err +// } +// +// // Remove the now-empty directory. +// err = os.Remove(path) +// if errors.Is(err, fs.ErrNotExist) { +// return nil +// } +// return err +//} +// +//func removeAllChildren(path string) error { +// entries, err := os.ReadDir(path) +// if err != nil { +// if errors.Is(err, fs.ErrNotExist) { +// return nil +// } +// return err +// } +// +// var firstErr error +// for _, entry := range entries { +// child := filepath.Join(path, entry.Name()) +// err := removeAll(child) +// if err != nil && firstErr == nil { +// firstErr = err +// } +// } +// return firstErr +//} func containsString(str string, a []string, caseSensitive bool) bool { if !caseSensitive { From 9729ffffe031e3eb427817a9a7e1152d8476ef88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Fri, 13 Mar 2026 18:52:15 +0100 Subject: [PATCH 02/16] Try using os.Rename and a reboot delete --- internal/pkg/agent/install/uninstall.go | 68 +------ internal/pkg/agent/install/uninstall_unix.go | 2 +- .../pkg/agent/install/uninstall_windows.go | 129 ++++---------- .../agent/install/uninstall_windows_test.go | 167 ++++++++++++++++-- 4 files changed, 181 insertions(+), 185 deletions(-) diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index e8772bb3977..e24c6232656 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -292,8 +292,8 @@ func RemovePath(path string) error { } if isBlockingOnExe(lastErr) { - // try to remove the blocking exe and try again to clean up the path - _ = removeBlockingExe(lastErr) + // try to move the blocking exe out of the way and try again + _ = removeBlockingExe(lastErr, path) } time.Sleep(500 * time.Millisecond) @@ -326,70 +326,6 @@ func RemoveBut(path string, bestEffort bool, exceptions ...string) error { return err } -// TODO: Replace this with a more robust and less mysterious approach. -// removeAll is a reimplementation of Go 1.24's os.RemoveAll. Go 1.25 switched -// to directory-relative unlinkat/openat syscalls (removeall_at.go) which on -// Windows use NtCreateFile with DELETE access — these are stricter about file -// state and fail on files that have been ADS-renamed. The simple path-based -// approach using os.Remove works correctly with the ADS rename trick that -// RemovePath uses to delete running executables on Windows. -// Taken from: https://cs.opensource.google/go/go/+/refs/tags/go1.24.13:src/os/removeall_noat.go;drc=a2baae6851a157d662dff7cc508659f66249698a;l=15 -// The implementation which breaks our install is here: -// https://cs.opensource.google/go/go/+/refs/tags/go1.25.8:src/os/removeall_at.go;drc=e81c624656e415626c7ac3a97768f5c2717979a4;l=15 -//func removeAll(path string) error { -// // Try simple remove first (handles files and empty directories). -// err := os.Remove(path) -// if err == nil || errors.Is(err, fs.ErrNotExist) { -// return nil -// } -// -// // Check if it's a directory. -// info, serr := os.Lstat(path) -// if serr != nil { -// if errors.Is(serr, fs.ErrNotExist) { -// return nil -// } -// return serr -// } -// if !info.IsDir() { -// // Not a directory — return the original Remove error. -// return err -// } -// -// // Remove directory contents recursively. -// err = removeAllChildren(path) -// if err != nil { -// return err -// } -// -// // Remove the now-empty directory. -// err = os.Remove(path) -// if errors.Is(err, fs.ErrNotExist) { -// return nil -// } -// return err -//} -// -//func removeAllChildren(path string) error { -// entries, err := os.ReadDir(path) -// if err != nil { -// if errors.Is(err, fs.ErrNotExist) { -// return nil -// } -// return err -// } -// -// var firstErr error -// for _, entry := range entries { -// child := filepath.Join(path, entry.Name()) -// err := removeAll(child) -// if err != nil && firstErr == nil { -// firstErr = err -// } -// } -// return firstErr -//} - func containsString(str string, a []string, caseSensitive bool) bool { if !caseSensitive { str = strings.ToLower(str) diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index 7a2f2d312af..c6d40b39e23 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -12,7 +12,7 @@ func isBlockingOnExe(_ error) bool { return false } -func removeBlockingExe(_ error) error { +func removeBlockingExe(_ error, _ string) error { return nil } diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index 40e8c7dab3e..b90dd035852 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -11,8 +11,8 @@ import ( "fmt" "io/fs" "os" + "path/filepath" "syscall" - "unsafe" "golang.org/x/sys/windows" ) @@ -39,37 +39,48 @@ func isRetryableError(err error) bool { return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION } -func removeBlockingExe(blockingErr error) error { +// removeBlockingExe moves a locked executable out of the way so that +// os.RemoveAll can remove the directory it lived in. On Windows a running exe +// cannot be deleted, but it *can* be renamed/moved (even across directories, +// as long as it stays on the same volume). After the move we schedule the temp +// file for deletion on the next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. +// +// rootPath is the top-level directory being removed (e.g. C:\Program Files\Elastic\Agent). +// The temp file is placed in the parent of rootPath so it is outside the tree +// being deleted but on the same volume. +func removeBlockingExe(blockingErr error, rootPath string) error { path, _ := getPathFromError(blockingErr) if path == "" { return nil } - // open handle for delete only - h, err := openDeleteHandle(path) + // Place the temp file in the parent of rootPath so it's outside the tree + // being removed but on the same volume (cross-volume rename won't work). + tmpDir := filepath.Dir(rootPath) + tmp, err := os.CreateTemp(tmpDir, ".elastic-agent-rm-*.exe") if err != nil { - return fmt.Errorf("failed to open handle for %q: %w", path, err) + return fmt.Errorf("failed to create temp file in %q: %w", tmpDir, err) } + tmpPath := tmp.Name() + _ = tmp.Close() - // rename handle - err = renameHandle(h) - _ = windows.CloseHandle(h) - if err != nil { - return fmt.Errorf("failed to rename handle for %q: %w", path, err) + // os.Rename uses MoveFileEx(MOVEFILE_REPLACE_EXISTING) under the hood. + if err := os.Rename(path, tmpPath); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("failed to rename %q to %q: %w", path, tmpPath, err) } - // re-open handle - h, err = openDeleteHandle(path) + // Schedule the temp file for deletion on next reboot. + tmpPathPtr, err := windows.UTF16PtrFromString(tmpPath) if err != nil { - return fmt.Errorf("failed to open handle after rename for %q: %w", path, err) + return fmt.Errorf("failed to convert temp path: %w", err) } - - // dispose of the handle - err = disposeHandle(h) - _ = windows.CloseHandle(h) - if err != nil { - return fmt.Errorf("failed to dispose handle for %q: %w", path, err) + if err := windows.MoveFileEx(tmpPathPtr, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT); err != nil { + // Non-fatal: the file is already out of the way, it just won't be + // auto-cleaned. Log-worthy but not worth failing the uninstall. + return fmt.Errorf("failed to schedule %q for deletion on reboot: %w", tmpPath, err) } + return nil } @@ -84,86 +95,6 @@ func getPathFromError(blockingErr error) (string, syscall.Errno) { return "", 0 } -func openDeleteHandle(path string) (windows.Handle, error) { - wPath, err := windows.UTF16PtrFromString(path) - if err != nil { - return 0, err - } - handle, err := windows.CreateFile( - wPath, - windows.DELETE, - 0, - nil, - windows.OPEN_EXISTING, - windows.FILE_ATTRIBUTE_NORMAL, - 0, - ) - if err != nil { - return 0, err - } - return handle, nil -} - -func renameHandle(hHandle windows.Handle) error { - wRename, err := windows.UTF16FromString(":agentrm") - if err != nil { - return err - } - - var rename fileRenameInfo - lpwStream := &wRename[0] - rename.FileNameLength = uint32(unsafe.Sizeof(lpwStream)) - - // RtlCopyMemory (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-rtlcopymemory) - // Return value - None - _, _, _ = windows.NewLazyDLL("kernel32.dll").NewProc("RtlCopyMemory").Call( - uintptr(unsafe.Pointer(&rename.FileName[0])), - uintptr(unsafe.Pointer(lpwStream)), - unsafe.Sizeof(lpwStream), - ) - - err = windows.SetFileInformationByHandle( - hHandle, - windows.FileRenameInfo, - (*byte)(unsafe.Pointer(&rename)), - uint32(unsafe.Sizeof(rename)+unsafe.Sizeof(lpwStream)), - ) - if err != nil { - return err - } - return nil -} - -func disposeHandle(hHandle windows.Handle) error { - var deleteFile fileDispositionInfo - deleteFile.DeleteFile = true - - err := windows.SetFileInformationByHandle( - hHandle, - windows.FileDispositionInfo, - (*byte)(unsafe.Pointer(&deleteFile)), - uint32(unsafe.Sizeof(deleteFile)), - ) - if err != nil { - return err - } - return nil -} - -type fileRenameInfo struct { - Union struct { - ReplaceIfExists bool - Flags uint32 - } - RootDirectory windows.Handle - FileNameLength uint32 - FileName [1]uint16 -} - -type fileDispositionInfo struct { - DeleteFile bool -} - // killNoneChildProcess provides a way of killing a process that is not started as a child of this process. // // On Windows when running in unprivileged mode the internal way that golang uses DuplicateHandle to perform the kill diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index 5c10fc0275b..a3c28ecd4cb 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -11,47 +11,176 @@ import ( "os" "os/exec" "path/filepath" + "strings" + "syscall" "testing" - "github.com/otiai10/copy" + cp "github.com/otiai10/copy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestRemovePath(t *testing.T) { - var ( +// startBlockingExe copies the testblocking binary into destDir and starts it. +// Returns the exec.Cmd and the path to the running exe. +func startBlockingExe(t *testing.T, destDir string) (*exec.Cmd, string) { + t.Helper() + + const ( pkgName = "testblocking" binaryName = pkgName + ".exe" ) - // Create a temporary directory that we can safely remove. The directory is created as a new - // sub-directory. This avoids having Microsoft Defender quarantine the file if it is exec'd from - // the default temporary directory. - destDir, err := os.MkdirTemp(pkgName, t.Name()) - require.NoError(t, err) - - // Copy the test executable to the new temporary directory. destpath, err := filepath.Abs(filepath.Join(destDir, binaryName)) - require.NoErrorf(t, err, "failed dest abs %s + %s", destDir, binaryName) + require.NoError(t, err) srcPath, err := filepath.Abs(filepath.Join(pkgName, binaryName)) - require.NoErrorf(t, err, "failed src abs %s + %s", pkgName, binaryName) + require.NoError(t, err) - err = copy.Copy(srcPath, destpath, copy.Options{Sync: true}) + err = cp.Copy(srcPath, destpath, cp.Options{Sync: true}) require.NoError(t, err) - // Execute the test executable asynchronously. - cmd := exec.Command(destpath) + cmd := exec.CommandContext(t.Context(), destpath) err = cmd.Start() require.NoError(t, err) - defer func() { + + t.Cleanup(func() { + // The process is killed automatically by CommandContext when the test + // context ends, so Kill may return "Access is denied" and Wait may + // return a non-zero exit code. We just need to reap the process. _ = cmd.Process.Kill() _ = cmd.Wait() - }() + }) + + return cmd, destpath +} + +// TestRemovePath verifies the full end-to-end: RemovePath successfully removes +// a directory containing a running executable. +func TestRemovePath(t *testing.T) { + destDir := filepath.Join(t.TempDir(), "target") + require.NoError(t, os.Mkdir(destDir, 0o755)) + + startBlockingExe(t, destDir) - // Ensure the directory containing the executable can be removed. - err = RemovePath(destDir) + err := RemovePath(destDir) assert.NoError(t, err) _, err = os.Stat(destDir) assert.ErrorIsf(t, err, fs.ErrNotExist, "path %q still exists after removal", destDir) } + +// TestRenameRunningExe verifies that os.Rename works on a running executable, +// which is the core assumption behind removeBlockingExe. +func TestRenameRunningExe(t *testing.T) { + destDir := filepath.Join(t.TempDir(), "target") + require.NoError(t, os.Mkdir(destDir, 0o755)) + + _, exePath := startBlockingExe(t, destDir) + + // Verify the exe cannot be deleted while running. + err := os.Remove(exePath) + require.Error(t, err, "os.Remove should fail on a running exe") + + // But it can be renamed. + tmpPath := exePath + ".moved" + err = os.Rename(exePath, tmpPath) + assert.NoError(t, err, "os.Rename should succeed on a running exe") + + // The original path no longer exists. + _, err = os.Stat(exePath) + assert.ErrorIs(t, err, fs.ErrNotExist) + + // The process is still running (rename doesn't kill it). + // The moved file exists at the new path. + _, err = os.Stat(tmpPath) + assert.NoError(t, err, "moved file should exist at new path") +} + +// TestRemoveAllSucceedsAfterRename verifies that once the running exe is +// moved out of the directory, os.RemoveAll can clean it up. +func TestRemoveAllSucceedsAfterRename(t *testing.T) { + tmpDir := t.TempDir() + destDir := filepath.Join(tmpDir, "target") + require.NoError(t, os.Mkdir(destDir, 0o755)) + + _, exePath := startBlockingExe(t, destDir) + + // RemoveAll fails while the exe is inside the directory. + err := os.RemoveAll(destDir) + require.Error(t, err, "RemoveAll should fail while running exe is in the directory") + + // Move the exe outside the directory (but inside tmpDir so t.TempDir cleans it up). + tmpPath := filepath.Join(tmpDir, ".test-rm.exe") + err = os.Rename(exePath, tmpPath) + require.NoError(t, err) + + // Now RemoveAll succeeds. + err = os.RemoveAll(destDir) + assert.NoError(t, err, "RemoveAll should succeed after moving the exe out") + + _, err = os.Stat(destDir) + assert.ErrorIs(t, err, fs.ErrNotExist) +} + +// TestRemoveBlockingExeMovesOutsideTree verifies that removeBlockingExe places +// the temp file in the parent of rootPath, not inside the tree being deleted. +func TestRemoveBlockingExeMovesOutsideTree(t *testing.T) { + tmpDir := t.TempDir() + destDir := filepath.Join(tmpDir, "target") + require.NoError(t, os.Mkdir(destDir, 0o755)) + + _, exePath := startBlockingExe(t, destDir) + + // Construct an error that matches what os.RemoveAll produces. + blockingErr := makeBlockingError(exePath) + + err := removeBlockingExe(blockingErr, destDir) + require.NoError(t, err, "removeBlockingExe should succeed") + + // The original path should be gone. + _, err = os.Stat(exePath) + assert.ErrorIs(t, err, fs.ErrNotExist, "original exe should no longer exist") + + // A temp file should exist in the parent of destDir (i.e. tmpDir). + entries, err := os.ReadDir(tmpDir) + require.NoError(t, err) + + var found bool + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".elastic-agent-rm-") && strings.HasSuffix(e.Name(), ".exe") { + found = true + break + } + } + assert.True(t, found, "temp file should exist in parent directory") +} + +// TestRemoveBlockingExeCleansTempOnFailure verifies that if os.Rename fails +// (e.g. path doesn't exist), the temp file is cleaned up. +func TestRemoveBlockingExeCleansTempOnFailure(t *testing.T) { + tmpDir := t.TempDir() + destDir := filepath.Join(tmpDir, "target") + require.NoError(t, os.Mkdir(destDir, 0o755)) + + // Use a non-existent path so the rename will fail. + fakePath := filepath.Join(destDir, "nonexistent.exe") + blockingErr := makeBlockingError(fakePath) + + err := removeBlockingExe(blockingErr, destDir) + assert.Error(t, err, "removeBlockingExe should fail when source doesn't exist") + + // No temp files should be left behind in the parent. + entries, err := os.ReadDir(tmpDir) + require.NoError(t, err) + + for _, e := range entries { + assert.False(t, + strings.HasPrefix(e.Name(), ".elastic-agent-rm-"), + "temp file %q should have been cleaned up", e.Name()) + } +} + +// makeBlockingError constructs an error matching what os.RemoveAll produces +// on Windows when it cannot delete a file due to a running exe. +func makeBlockingError(path string) error { + return &fs.PathError{Op: "unlinkat", Path: path, Err: syscall.ERROR_ACCESS_DENIED} +} From 7ecd612b3ce7338dbec15335c68931395feb6bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Tue, 17 Mar 2026 12:01:12 +0100 Subject: [PATCH 03/16] Put the moved file in Temp by default --- internal/pkg/agent/install/uninstall.go | 3 +- internal/pkg/agent/install/uninstall_unix.go | 9 +++++- .../pkg/agent/install/uninstall_windows.go | 29 +++++++++++++------ .../agent/install/uninstall_windows_test.go | 17 +++++------ 4 files changed, 38 insertions(+), 20 deletions(-) diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index e24c6232656..d6567fdd2ee 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -282,6 +282,7 @@ func checkForUnprivilegedVault(ctx context.Context, opts ...vault.OptionFunc) (b // seconds if it keeps getting that error. func RemovePath(path string) error { const arbitraryTimeout = 60 * time.Second + tmpDir := tempDirOnSameVolume(path) start := time.Now() var lastErr error for time.Since(start) <= arbitraryTimeout { @@ -293,7 +294,7 @@ func RemovePath(path string) error { if isBlockingOnExe(lastErr) { // try to move the blocking exe out of the way and try again - _ = removeBlockingExe(lastErr, path) + _ = removeBlockingExe(lastErr, tmpDir) } time.Sleep(500 * time.Millisecond) diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index c6d40b39e23..1a5fd4f8567 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -6,7 +6,10 @@ package install -import "os" +import ( + "os" + "path/filepath" +) func isBlockingOnExe(_ error) bool { return false @@ -20,6 +23,10 @@ func isRetryableError(_ error) bool { return false } +func tempDirOnSameVolume(path string) string { + return filepath.Dir(path) +} + // killNoneChildProcess provides a way of killing a process that is not started as a child of this process. // // On Unix systems it just calls the native golang kill. diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index b90dd035852..f6287af595a 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -12,6 +12,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "syscall" "golang.org/x/sys/windows" @@ -45,21 +46,18 @@ func isRetryableError(err error) bool { // as long as it stays on the same volume). After the move we schedule the temp // file for deletion on the next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. // -// rootPath is the top-level directory being removed (e.g. C:\Program Files\Elastic\Agent). -// The temp file is placed in the parent of rootPath so it is outside the tree -// being deleted but on the same volume. -func removeBlockingExe(blockingErr error, rootPath string) error { +// tempDir is the directory where the temp file will be created. It must be on +// the same volume as the blocked file (cross-volume rename won't work for +// in-use files). +func removeBlockingExe(blockingErr error, tempDir string) error { path, _ := getPathFromError(blockingErr) if path == "" { return nil } - // Place the temp file in the parent of rootPath so it's outside the tree - // being removed but on the same volume (cross-volume rename won't work). - tmpDir := filepath.Dir(rootPath) - tmp, err := os.CreateTemp(tmpDir, ".elastic-agent-rm-*.exe") + tmp, err := os.CreateTemp(tempDir, ".elastic-agent-rm-*.exe") if err != nil { - return fmt.Errorf("failed to create temp file in %q: %w", tmpDir, err) + return fmt.Errorf("failed to create temp file in %q: %w", tempDir, err) } tmpPath := tmp.Name() _ = tmp.Close() @@ -84,6 +82,19 @@ func removeBlockingExe(blockingErr error, rootPath string) error { return nil } +// tempDirOnSameVolume returns os.TempDir() if it is on the same volume as +// path, otherwise falls back to the parent of path. Rename/move of an in-use +// file only works within the same volume. +func tempDirOnSameVolume(path string) string { + sysTemp := os.TempDir() + pathVol := filepath.VolumeName(path) + tempVol := filepath.VolumeName(sysTemp) + if strings.EqualFold(pathVol, tempVol) { + return sysTemp + } + return filepath.Dir(path) +} + func getPathFromError(blockingErr error) (string, syscall.Errno) { var perr *fs.PathError if errors.As(blockingErr, &perr) { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index a3c28ecd4cb..a22986fbc52 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -122,7 +122,7 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { } // TestRemoveBlockingExeMovesOutsideTree verifies that removeBlockingExe places -// the temp file in the parent of rootPath, not inside the tree being deleted. +// the temp file in the specified temp directory, outside the tree being deleted. func TestRemoveBlockingExeMovesOutsideTree(t *testing.T) { tmpDir := t.TempDir() destDir := filepath.Join(tmpDir, "target") @@ -130,28 +130,27 @@ func TestRemoveBlockingExeMovesOutsideTree(t *testing.T) { _, exePath := startBlockingExe(t, destDir) - // Construct an error that matches what os.RemoveAll produces. blockingErr := makeBlockingError(exePath) - err := removeBlockingExe(blockingErr, destDir) + err := removeBlockingExe(blockingErr, tmpDir) require.NoError(t, err, "removeBlockingExe should succeed") // The original path should be gone. _, err = os.Stat(exePath) assert.ErrorIs(t, err, fs.ErrNotExist, "original exe should no longer exist") - // A temp file should exist in the parent of destDir (i.e. tmpDir). + // The temp file should exist in tmpDir. entries, err := os.ReadDir(tmpDir) require.NoError(t, err) - var found bool + var found string for _, e := range entries { if strings.HasPrefix(e.Name(), ".elastic-agent-rm-") && strings.HasSuffix(e.Name(), ".exe") { - found = true + found = filepath.Join(tmpDir, e.Name()) break } } - assert.True(t, found, "temp file should exist in parent directory") + assert.NotEmpty(t, found, "temp file should exist in %s", tmpDir) } // TestRemoveBlockingExeCleansTempOnFailure verifies that if os.Rename fails @@ -165,10 +164,10 @@ func TestRemoveBlockingExeCleansTempOnFailure(t *testing.T) { fakePath := filepath.Join(destDir, "nonexistent.exe") blockingErr := makeBlockingError(fakePath) - err := removeBlockingExe(blockingErr, destDir) + err := removeBlockingExe(blockingErr, tmpDir) assert.Error(t, err, "removeBlockingExe should fail when source doesn't exist") - // No temp files should be left behind in the parent. + // No temp files should be left behind. entries, err := os.ReadDir(tmpDir) require.NoError(t, err) From 50ac81d782eeee0b890200cbf00436ff7515c7a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Mon, 30 Mar 2026 14:57:18 +0200 Subject: [PATCH 04/16] Leave the executable in the root dir --- internal/pkg/agent/install/uninstall.go | 15 +++- internal/pkg/agent/install/uninstall_unix.go | 13 +-- .../pkg/agent/install/uninstall_windows.go | 68 ++++------------ .../agent/install/uninstall_windows_test.go | 80 ++++++++----------- 4 files changed, 65 insertions(+), 111 deletions(-) diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index d6567fdd2ee..728ab5e3d53 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -282,7 +282,6 @@ func checkForUnprivilegedVault(ctx context.Context, opts ...vault.OptionFunc) (b // seconds if it keeps getting that error. func RemovePath(path string) error { const arbitraryTimeout = 60 * time.Second - tmpDir := tempDirOnSameVolume(path) start := time.Now() var lastErr error for time.Since(start) <= arbitraryTimeout { @@ -293,8 +292,18 @@ func RemovePath(path string) error { } if isBlockingOnExe(lastErr) { - // try to move the blocking exe out of the way and try again - _ = removeBlockingExe(lastErr, tmpDir) + // Schedule the blocking exe and its ancestor directories for + // deletion on reboot. os.RemoveAll already deleted everything + // it could; only the exe and its parent dirs remain. + scheduleDeleteOnReboot(lastErr, path) + // One more RemoveAll to clean up directories that may now be + // empty after the previous pass. + if err := os.RemoveAll(path); err == nil { + return nil + } + // The exe and its ancestor dirs are still present but scheduled + // for reboot deletion. Consider this a success. + return nil } time.Sleep(500 * time.Millisecond) diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index 1a5fd4f8567..1977f03647c 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -6,27 +6,18 @@ package install -import ( - "os" - "path/filepath" -) +import "os" func isBlockingOnExe(_ error) bool { return false } -func removeBlockingExe(_ error, _ string) error { - return nil -} +func scheduleDeleteOnReboot(_ error, _ string) {} func isRetryableError(_ error) bool { return false } -func tempDirOnSameVolume(path string) string { - return filepath.Dir(path) -} - // killNoneChildProcess provides a way of killing a process that is not started as a child of this process. // // On Unix systems it just calls the native golang kill. diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index f6287af595a..9a8929d63bf 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -8,11 +8,8 @@ package install import ( "errors" - "fmt" "io/fs" "os" - "path/filepath" - "strings" "syscall" "golang.org/x/sys/windows" @@ -40,59 +37,28 @@ func isRetryableError(err error) bool { return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION } -// removeBlockingExe moves a locked executable out of the way so that -// os.RemoveAll can remove the directory it lived in. On Windows a running exe -// cannot be deleted, but it *can* be renamed/moved (even across directories, -// as long as it stays on the same volume). After the move we schedule the temp -// file for deletion on the next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. -// -// tempDir is the directory where the temp file will be created. It must be on -// the same volume as the blocked file (cross-volume rename won't work for -// in-use files). -func removeBlockingExe(blockingErr error, tempDir string) error { - path, _ := getPathFromError(blockingErr) - if path == "" { - return nil - } - - tmp, err := os.CreateTemp(tempDir, ".elastic-agent-rm-*.exe") - if err != nil { - return fmt.Errorf("failed to create temp file in %q: %w", tempDir, err) - } - tmpPath := tmp.Name() - _ = tmp.Close() - - // os.Rename uses MoveFileEx(MOVEFILE_REPLACE_EXISTING) under the hood. - if err := os.Rename(path, tmpPath); err != nil { - _ = os.Remove(tmpPath) - return fmt.Errorf("failed to rename %q to %q: %w", path, tmpPath, err) - } - - // Schedule the temp file for deletion on next reboot. - tmpPathPtr, err := windows.UTF16PtrFromString(tmpPath) - if err != nil { - return fmt.Errorf("failed to convert temp path: %w", err) - } - if err := windows.MoveFileEx(tmpPathPtr, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT); err != nil { - // Non-fatal: the file is already out of the way, it just won't be - // auto-cleaned. Log-worthy but not worth failing the uninstall. - return fmt.Errorf("failed to schedule %q for deletion on reboot: %w", tmpPath, err) +// scheduleDeleteOnReboot marks the blocked file for deletion on the next +// reboot using MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. The file is not moved +// or renamed, which avoids triggering EDR software. The ancestor directories +// are left in place. +func scheduleDeleteOnReboot(blockingErr error, _ string) { + blockedPath, _ := getPathFromError(blockingErr) + if blockedPath == "" { + return } - return nil + _ = markDeleteOnReboot(blockedPath) } -// tempDirOnSameVolume returns os.TempDir() if it is on the same volume as -// path, otherwise falls back to the parent of path. Rename/move of an in-use -// file only works within the same volume. -func tempDirOnSameVolume(path string) string { - sysTemp := os.TempDir() - pathVol := filepath.VolumeName(path) - tempVol := filepath.VolumeName(sysTemp) - if strings.EqualFold(pathVol, tempVol) { - return sysTemp +// markDeleteOnReboot schedules a file or directory for deletion on the next +// Windows reboot. This only writes a registry entry +// (PendingFileRenameOperations) and does not touch the file itself. +func markDeleteOnReboot(path string) error { + p, err := windows.UTF16PtrFromString(path) + if err != nil { + return err } - return filepath.Dir(path) + return windows.MoveFileEx(p, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT) } func getPathFromError(blockingErr error) (string, syscall.Errno) { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index a22986fbc52..25c11404960 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -11,7 +11,6 @@ import ( "os" "os/exec" "path/filepath" - "strings" "syscall" "testing" @@ -55,7 +54,8 @@ func startBlockingExe(t *testing.T, destDir string) (*exec.Cmd, string) { } // TestRemovePath verifies the full end-to-end: RemovePath successfully removes -// a directory containing a running executable. +// a directory containing a running executable (or schedules leftovers for +// reboot deletion and returns nil). func TestRemovePath(t *testing.T) { destDir := filepath.Join(t.TempDir(), "target") require.NoError(t, os.Mkdir(destDir, 0o755)) @@ -64,12 +64,9 @@ func TestRemovePath(t *testing.T) { err := RemovePath(destDir) assert.NoError(t, err) - _, err = os.Stat(destDir) - assert.ErrorIsf(t, err, fs.ErrNotExist, "path %q still exists after removal", destDir) } -// TestRenameRunningExe verifies that os.Rename works on a running executable, -// which is the core assumption behind removeBlockingExe. +// TestRenameRunningExe verifies that os.Rename works on a running executable. func TestRenameRunningExe(t *testing.T) { destDir := filepath.Join(t.TempDir(), "target") require.NoError(t, os.Mkdir(destDir, 0o755)) @@ -90,7 +87,6 @@ func TestRenameRunningExe(t *testing.T) { assert.ErrorIs(t, err, fs.ErrNotExist) // The process is still running (rename doesn't kill it). - // The moved file exists at the new path. _, err = os.Stat(tmpPath) assert.NoError(t, err, "moved file should exist at new path") } @@ -108,7 +104,7 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { err := os.RemoveAll(destDir) require.Error(t, err, "RemoveAll should fail while running exe is in the directory") - // Move the exe outside the directory (but inside tmpDir so t.TempDir cleans it up). + // Move the exe outside the directory. tmpPath := filepath.Join(tmpDir, ".test-rm.exe") err = os.Rename(exePath, tmpPath) require.NoError(t, err) @@ -121,9 +117,10 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { assert.ErrorIs(t, err, fs.ErrNotExist) } -// TestRemoveBlockingExeMovesOutsideTree verifies that removeBlockingExe places -// the temp file in the specified temp directory, outside the tree being deleted. -func TestRemoveBlockingExeMovesOutsideTree(t *testing.T) { +// TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot marks the +// blocked file and its ancestor directories for reboot deletion without moving +// the file. +func TestScheduleDeleteOnReboot(t *testing.T) { tmpDir := t.TempDir() destDir := filepath.Join(tmpDir, "target") require.NoError(t, os.Mkdir(destDir, 0o755)) @@ -131,51 +128,42 @@ func TestRemoveBlockingExeMovesOutsideTree(t *testing.T) { _, exePath := startBlockingExe(t, destDir) blockingErr := makeBlockingError(exePath) + scheduleDeleteOnReboot(blockingErr, destDir) - err := removeBlockingExe(blockingErr, tmpDir) - require.NoError(t, err, "removeBlockingExe should succeed") - - // The original path should be gone. - _, err = os.Stat(exePath) - assert.ErrorIs(t, err, fs.ErrNotExist, "original exe should no longer exist") - - // The temp file should exist in tmpDir. - entries, err := os.ReadDir(tmpDir) - require.NoError(t, err) - - var found string - for _, e := range entries { - if strings.HasPrefix(e.Name(), ".elastic-agent-rm-") && strings.HasSuffix(e.Name(), ".exe") { - found = filepath.Join(tmpDir, e.Name()) - break - } - } - assert.NotEmpty(t, found, "temp file should exist in %s", tmpDir) + // The file should still be in its original location (no move). + _, err := os.Stat(exePath) + assert.NoError(t, err, "exe should still exist at original path after scheduling reboot deletion") } -// TestRemoveBlockingExeCleansTempOnFailure verifies that if os.Rename fails -// (e.g. path doesn't exist), the temp file is cleaned up. -func TestRemoveBlockingExeCleansTempOnFailure(t *testing.T) { +// TestRemoveAllDeletesEverythingExceptBlockingExe verifies that os.RemoveAll +// deletes all files it can and only fails on the running exe. +func TestRemoveAllDeletesEverythingExceptBlockingExe(t *testing.T) { tmpDir := t.TempDir() destDir := filepath.Join(tmpDir, "target") require.NoError(t, os.Mkdir(destDir, 0o755)) - // Use a non-existent path so the rename will fail. - fakePath := filepath.Join(destDir, "nonexistent.exe") - blockingErr := makeBlockingError(fakePath) + // Create some additional files alongside the exe. + require.NoError(t, os.WriteFile(filepath.Join(destDir, "config.yml"), []byte("test"), 0o644)) + subDir := filepath.Join(destDir, "subdir") + require.NoError(t, os.Mkdir(subDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(subDir, "data.txt"), []byte("test"), 0o644)) - err := removeBlockingExe(blockingErr, tmpDir) - assert.Error(t, err, "removeBlockingExe should fail when source doesn't exist") + _, exePath := startBlockingExe(t, destDir) - // No temp files should be left behind. - entries, err := os.ReadDir(tmpDir) - require.NoError(t, err) + // RemoveAll should fail on the exe but delete everything else. + err := os.RemoveAll(destDir) + require.Error(t, err, "RemoveAll should fail due to running exe") + + // The exe should still exist. + _, err = os.Stat(exePath) + assert.NoError(t, err, "exe should still exist") + + // Other files should be gone. + _, err = os.Stat(filepath.Join(destDir, "config.yml")) + assert.ErrorIs(t, err, fs.ErrNotExist, "config.yml should be deleted") - for _, e := range entries { - assert.False(t, - strings.HasPrefix(e.Name(), ".elastic-agent-rm-"), - "temp file %q should have been cleaned up", e.Name()) - } + _, err = os.Stat(filepath.Join(subDir, "data.txt")) + assert.ErrorIs(t, err, fs.ErrNotExist, "data.txt should be deleted") } // makeBlockingError constructs an error matching what os.RemoveAll produces From 4b703b1e90ebe9210e0c9d72b2b69777ba69cac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Mon, 30 Mar 2026 15:09:18 +0200 Subject: [PATCH 05/16] Remove the top path during install --- internal/pkg/agent/install/install.go | 8 +++++++ internal/pkg/agent/install/install_test.go | 28 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index 8adac8ca052..626a251d2f6 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -209,6 +209,14 @@ func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt Pr // setup the basic topPath, and the .installed file func setupInstallPath(topPath string, ownership utils.FileOwner) error { + // Clean up any remnants from a previous installation. On Windows, + // uninstall may leave behind the running executable (scheduled for + // deletion on reboot via MoveFileEx). By the time a new install runs + // the old process is gone, so these files can now be deleted normally. + if err := os.RemoveAll(topPath); err != nil { + return errors.New(err, fmt.Sprintf("failed to clean up previous installation at (%s)", topPath), errors.M("directory", topPath)) + } + // ensure parent directory exists err := os.MkdirAll(filepath.Dir(topPath), 0755) if err != nil { diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index 795dcfb0416..6004c645f2c 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -6,6 +6,7 @@ package install import ( "fmt" + "io/fs" "os" "path/filepath" "testing" @@ -229,3 +230,30 @@ func TestSetupInstallPath(t *testing.T) { markerFilePath := filepath.Join(tmpdir, paths.MarkerFileName) require.FileExists(t, markerFilePath) } + +func TestSetupInstallPathCleansUpPreviousInstall(t *testing.T) { + tmpdir := t.TempDir() + topPath := filepath.Join(tmpdir, "Elastic", "Agent") + + // Simulate leftover files from a previous installation. + require.NoError(t, os.MkdirAll(filepath.Join(topPath, "data", "elastic-agent-old"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(topPath, "data", "elastic-agent-old", "elastic-agent.exe"), []byte("old binary"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(topPath, "some-config.yml"), []byte("old config"), 0o644)) + + ownership, err := utils.CurrentFileOwner() + require.NoError(t, err) + + err = setupInstallPath(topPath, ownership) + require.NoError(t, err) + + // The install marker should exist (fresh install). + markerFilePath := filepath.Join(topPath, paths.MarkerFileName) + require.FileExists(t, markerFilePath) + + // Leftover files from the previous installation should be gone. + _, err = os.Stat(filepath.Join(topPath, "data")) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover data directory should be removed") + + _, err = os.Stat(filepath.Join(topPath, "some-config.yml")) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover config file should be removed") +} From ca77d56498cd10f70a496849048f08484fa359c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Tue, 31 Mar 2026 17:49:26 +0200 Subject: [PATCH 06/16] Improve logging --- .../application/upgrade/manual_rollback.go | 2 +- .../pkg/agent/application/upgrade/rollback.go | 2 +- internal/pkg/agent/cmd/uninstall.go | 2 +- internal/pkg/agent/install/install.go | 17 +++--- internal/pkg/agent/install/install_test.go | 34 +++++++---- internal/pkg/agent/install/uninstall.go | 36 ++++++----- internal/pkg/agent/install/uninstall_unix.go | 10 ++- .../pkg/agent/install/uninstall_windows.go | 61 ++++++++++++++++--- .../agent/install/uninstall_windows_test.go | 33 +++++++--- pkg/testing/fixture.go | 3 +- 10 files changed, 144 insertions(+), 56 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/manual_rollback.go b/internal/pkg/agent/application/upgrade/manual_rollback.go index c16830af0eb..fa2165976f9 100644 --- a/internal/pkg/agent/application/upgrade/manual_rollback.go +++ b/internal/pkg/agent/application/upgrade/manual_rollback.go @@ -347,7 +347,7 @@ func CleanAvailableRollbacks(log *logger.Logger, source availableRollbacksSource if filter(log, now, versionedHome, ttlMarker) { log.Debugf("cleaning up rollback in %q", versionedHome) - if cleanupErr := install.RemoveBut(versionedHomeAbsPath, true); cleanupErr != nil { + if cleanupErr := install.RemoveBut(log, versionedHomeAbsPath, true); cleanupErr != nil { aggregateErr = errors.Join(aggregateErr, fmt.Errorf("removing directory %q: %w", versionedHomeAbsPath, cleanupErr)) } else { if removeErr := source.Remove(versionedHome); removeErr != nil { diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 380e074ab03..aaa75dcd6bb 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -205,7 +205,7 @@ func cleanup(log *logger.Logger, topDirPath string, removeMarker, keepLogs bool, if keepLogs { ignoredDirs = append(ignoredDirs, "logs") } - if cleanupErr := install.RemoveBut(hashedDir, true, ignoredDirs...); cleanupErr != nil { + if cleanupErr := install.RemoveBut(log, hashedDir, true, ignoredDirs...); cleanupErr != nil { cumulativeError = goerrors.Join(cumulativeError, cleanupErr) } } diff --git a/internal/pkg/agent/cmd/uninstall.go b/internal/pkg/agent/cmd/uninstall.go index fca0bc47f2b..6b15d6b7670 100644 --- a/internal/pkg/agent/cmd/uninstall.go +++ b/internal/pkg/agent/cmd/uninstall.go @@ -108,6 +108,6 @@ func uninstallCmd(streams *cli.IOStreams, cmd *cobra.Command) error { _ = progBar.Exit() fmt.Fprintf(streams.Out, "\n%s has been uninstalled.\n", paths.ServiceDisplayName()) - _ = install.RemovePath(paths.Top()) + _ = install.RemovePath(log, paths.Top()) return nil } diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index 626a251d2f6..80030b407e8 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -64,7 +64,7 @@ func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt Pr } } - err = setupInstallPath(topPath, ownership) + err = setupInstallPath(log, topPath, ownership) if err != nil { return utils.FileOwner{}, fmt.Errorf("error setting up install path: %w", err) } @@ -208,13 +208,14 @@ func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt Pr } // setup the basic topPath, and the .installed file -func setupInstallPath(topPath string, ownership utils.FileOwner) error { - // Clean up any remnants from a previous installation. On Windows, - // uninstall may leave behind the running executable (scheduled for - // deletion on reboot via MoveFileEx). By the time a new install runs - // the old process is gone, so these files can now be deleted normally. - if err := os.RemoveAll(topPath); err != nil { - return errors.New(err, fmt.Sprintf("failed to clean up previous installation at (%s)", topPath), errors.M("directory", topPath)) +func setupInstallPath(log *logp.Logger, topPath string, ownership utils.FileOwner) error { + // Clean up renamed executables left behind by a previous uninstall. On + // Windows, uninstall renames the running executable (with a recognizable + // prefix) and schedules it for deletion on reboot. If the system was not + // rebooted before reinstalling, these files are still present but no + // longer locked, so we can delete them now. + if err := cleanupLeftoverRenames(topPath); err != nil { + log.Warnf("Failed to clean up leftover files from a previous installation in %q: %v. You may need to delete them manually.", topPath, err) } // ensure parent directory exists diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index 6004c645f2c..38ed5c0fa33 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -9,6 +9,7 @@ import ( "io/fs" "os" "path/filepath" + "runtime" "testing" "github.com/jaypipes/ghw" @@ -16,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" v1 "github.com/elastic/elastic-agent/pkg/api/v1" "github.com/elastic/elastic-agent/pkg/utils" @@ -225,35 +227,43 @@ func TestSetupInstallPath(t *testing.T) { tmpdir := t.TempDir() ownership, err := utils.CurrentFileOwner() require.NoError(t, err) - err = setupInstallPath(tmpdir, ownership) + err = setupInstallPath(logp.L(), tmpdir, ownership) require.NoError(t, err) markerFilePath := filepath.Join(tmpdir, paths.MarkerFileName) require.FileExists(t, markerFilePath) } -func TestSetupInstallPathCleansUpPreviousInstall(t *testing.T) { +func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { tmpdir := t.TempDir() topPath := filepath.Join(tmpdir, "Elastic", "Agent") - // Simulate leftover files from a previous installation. - require.NoError(t, os.MkdirAll(filepath.Join(topPath, "data", "elastic-agent-old"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(topPath, "data", "elastic-agent-old", "elastic-agent.exe"), []byte("old binary"), 0o644)) - require.NoError(t, os.WriteFile(filepath.Join(topPath, "some-config.yml"), []byte("old config"), 0o644)) + // Simulate leftover renamed executable from a previous uninstall. + dataDir := filepath.Join(topPath, "data", "elastic-agent-old") + require.NoError(t, os.MkdirAll(dataDir, 0o755)) + leftoverFile := filepath.Join(dataDir, ".elastic-agent-leftover.exe") + require.NoError(t, os.WriteFile(leftoverFile, []byte("old binary"), 0o644)) + + // Also create a normal file that should NOT be deleted. + normalFile := filepath.Join(topPath, "some-config.yml") + require.NoError(t, os.WriteFile(normalFile, []byte("config"), 0o644)) ownership, err := utils.CurrentFileOwner() require.NoError(t, err) - err = setupInstallPath(topPath, ownership) + err = setupInstallPath(logp.L(), topPath, ownership) require.NoError(t, err) // The install marker should exist (fresh install). markerFilePath := filepath.Join(topPath, paths.MarkerFileName) require.FileExists(t, markerFilePath) - // Leftover files from the previous installation should be gone. - _, err = os.Stat(filepath.Join(topPath, "data")) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover data directory should be removed") + // Leftover renamed executable should be cleaned up (Windows only). + if runtime.GOOS == "windows" { + _, err = os.Stat(leftoverFile) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover renamed exe should be removed") + } - _, err = os.Stat(filepath.Join(topPath, "some-config.yml")) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover config file should be removed") + // Normal files should still be present. + _, err = os.Stat(normalFile) + assert.NoError(t, err, "normal config file should not be deleted") } diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index 728ab5e3d53..cb9283558d0 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -155,7 +155,7 @@ func Uninstall(ctx context.Context, cfgFile, topPath, uninstallToken string, log // remove existing directory pt.Describe("Removing install directory") - err = RemovePath(topPath) + err = RemovePath(log, topPath) if err != nil { pt.Describe("Failed to remove install directory") return aerrors.New( @@ -280,29 +280,35 @@ func checkForUnprivilegedVault(ctx context.Context, opts ...vault.OptionFunc) (b // On Windows it is possible that a removal can spuriously error due // to an ERROR_SHARING_VIOLATION. RemovePath will retry up to 2 // seconds if it keeps getting that error. -func RemovePath(path string) error { +func RemovePath(log *logp.Logger, path string) error { const arbitraryTimeout = 60 * time.Second start := time.Now() var lastErr error + attempt := 0 for time.Since(start) <= arbitraryTimeout { + attempt++ lastErr = os.RemoveAll(path) if lastErr == nil || !isRetryableError(lastErr) { return lastErr } + log.Debugf("RemovePath attempt %d failed after %s: %v", attempt, time.Since(start).Truncate(time.Millisecond), lastErr) + if isBlockingOnExe(lastErr) { - // Schedule the blocking exe and its ancestor directories for - // deletion on reboot. os.RemoveAll already deleted everything - // it could; only the exe and its parent dirs remain. - scheduleDeleteOnReboot(lastErr, path) - // One more RemoveAll to clean up directories that may now be - // empty after the previous pass. - if err := os.RemoveAll(path); err == nil { - return nil + // Rename the blocked exe in place (recognizable prefix) and + // schedule it for deletion on reboot. os.RemoveAll already + // deleted everything it could; only the exe and its ancestor + // dirs remain. + if err := scheduleDeleteOnReboot(log, lastErr, path); err != nil { + log.Errorf("Failed to schedule blocked file for removal: %v. You may need to manually delete %q.", err, path) + return fmt.Errorf("failed to handle blocked executable in %q: %w", path, err) + } + // Try RemoveAll again — the original exe path is now free + // after the rename, so more of the tree may be removable. + if err := os.RemoveAll(path); err != nil { + log.Warnf("Some files in %q could not be removed and are scheduled for deletion on reboot or next install.", path) } - // The exe and its ancestor dirs are still present but scheduled - // for reboot deletion. Consider this a success. return nil } @@ -312,9 +318,9 @@ func RemovePath(path string) error { return fmt.Errorf("timed out while removing %q. Last error: %w", path, lastErr) } -func RemoveBut(path string, bestEffort bool, exceptions ...string) error { +func RemoveBut(log *logp.Logger, path string, bestEffort bool, exceptions ...string) error { if len(exceptions) == 0 { - return RemovePath(path) + return RemovePath(log, path) } files, err := os.ReadDir(path) @@ -327,7 +333,7 @@ func RemoveBut(path string, bestEffort bool, exceptions ...string) error { continue } - err = RemovePath(filepath.Join(path, f.Name())) + err = RemovePath(log, filepath.Join(path, f.Name())) if !bestEffort && err != nil { return fmt.Errorf("error removing path %s: %w", f.Name(), err) } diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index 1977f03647c..52a631ba41a 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -6,13 +6,19 @@ package install -import "os" +import ( + "os" + + "github.com/elastic/elastic-agent-libs/logp" +) func isBlockingOnExe(_ error) bool { return false } -func scheduleDeleteOnReboot(_ error, _ string) {} +func scheduleDeleteOnReboot(_ *logp.Logger, _ error, _ string) error { return nil } + +func cleanupLeftoverRenames(_ string) error { return nil } func isRetryableError(_ error) bool { return false diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index 9a8929d63bf..fa582e5a524 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -8,13 +8,23 @@ package install import ( "errors" + "fmt" "io/fs" "os" + "path/filepath" + "strings" "syscall" + "time" "golang.org/x/sys/windows" + + "github.com/elastic/elastic-agent-libs/logp" ) +// leftoverPrefix is used to rename blocked executables during uninstall. +// Files with this prefix are cleaned up on the next install. +const leftoverPrefix = ".elastic-agent-leftover" + func isBlockingOnExe(err error) bool { if err == nil { return false @@ -37,17 +47,35 @@ func isRetryableError(err error) bool { return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION } -// scheduleDeleteOnReboot marks the blocked file for deletion on the next -// reboot using MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. The file is not moved -// or renamed, which avoids triggering EDR software. The ancestor directories -// are left in place. -func scheduleDeleteOnReboot(blockingErr error, _ string) { +// scheduleDeleteOnReboot renames the blocked executable in place (same +// directory, recognizable prefix) and schedules it for deletion on the next +// reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. Renaming in-place avoids +// triggering EDR software. On the next install, any leftover files matching +// the prefix are cleaned up by cleanupLeftoverRenames. +func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, _ string) error { blockedPath, _ := getPathFromError(blockingErr) if blockedPath == "" { - return + return nil + } + + dir := filepath.Dir(blockedPath) + ext := filepath.Ext(blockedPath) + renamed := filepath.Join(dir, fmt.Sprintf("%s-%d%s", leftoverPrefix, time.Now().UnixNano(), ext)) + + if err := os.Rename(blockedPath, renamed); err != nil { + return fmt.Errorf("failed to rename %q to %q: %w", blockedPath, renamed, err) + } + log.Infof("Renamed blocked executable %q to %q", blockedPath, renamed) + + if err := markDeleteOnReboot(renamed); err != nil { + log.Warnf("Failed to schedule %q for deletion on reboot: %v. The file will be cleaned up on next install.", renamed, err) + // Not fatal — the file is already renamed out of the way and will + // be cleaned up by cleanupLeftoverRenames on next install. + } else { + log.Infof("Scheduled %q for deletion on reboot", renamed) } - _ = markDeleteOnReboot(blockedPath) + return nil } // markDeleteOnReboot schedules a file or directory for deletion on the next @@ -61,6 +89,25 @@ func markDeleteOnReboot(path string) error { return windows.MoveFileEx(p, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT) } +// cleanupLeftoverRenames walks topPath and removes any files left behind by +// a previous uninstall's scheduleDeleteOnReboot (files matching leftoverPrefix). +// By the time a new install runs, the old process is gone and these files can +// be deleted normally. +func cleanupLeftoverRenames(topPath string) error { + return filepath.WalkDir(topPath, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return nil // best-effort: skip inaccessible entries + } + if d.IsDir() { + return nil + } + if strings.HasPrefix(d.Name(), leftoverPrefix) { + _ = os.Remove(path) + } + return nil + }) +} + func getPathFromError(blockingErr error) (string, syscall.Errno) { var perr *fs.PathError if errors.As(blockingErr, &perr) { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index 25c11404960..af84251c452 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -11,12 +11,15 @@ import ( "os" "os/exec" "path/filepath" + "strings" "syscall" "testing" cp "github.com/otiai10/copy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/elastic/elastic-agent-libs/logp" ) // startBlockingExe copies the testblocking binary into destDir and starts it. @@ -62,7 +65,7 @@ func TestRemovePath(t *testing.T) { startBlockingExe(t, destDir) - err := RemovePath(destDir) + err := RemovePath(logp.L(), destDir) assert.NoError(t, err) } @@ -117,9 +120,9 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { assert.ErrorIs(t, err, fs.ErrNotExist) } -// TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot marks the -// blocked file and its ancestor directories for reboot deletion without moving -// the file. +// TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot renames the +// blocked executable in place with the leftover prefix and keeps it in the +// same directory. func TestScheduleDeleteOnReboot(t *testing.T) { tmpDir := t.TempDir() destDir := filepath.Join(tmpDir, "target") @@ -128,11 +131,25 @@ func TestScheduleDeleteOnReboot(t *testing.T) { _, exePath := startBlockingExe(t, destDir) blockingErr := makeBlockingError(exePath) - scheduleDeleteOnReboot(blockingErr, destDir) + err := scheduleDeleteOnReboot(logp.L(), blockingErr, destDir) + require.NoError(t, err) + + // The original path should be gone (renamed). + _, err = os.Stat(exePath) + assert.ErrorIs(t, err, fs.ErrNotExist, "original exe path should no longer exist") + + // A file with the leftover prefix should exist in the same directory. + entries, err := os.ReadDir(destDir) + require.NoError(t, err) - // The file should still be in its original location (no move). - _, err := os.Stat(exePath) - assert.NoError(t, err, "exe should still exist at original path after scheduling reboot deletion") + var found bool + for _, e := range entries { + if strings.HasPrefix(e.Name(), leftoverPrefix) { + found = true + break + } + } + assert.True(t, found, "renamed file with prefix %q should exist in %s", leftoverPrefix, destDir) } // TestRemoveAllDeletesEverythingExceptBlockingExe verifies that os.RemoveAll diff --git a/pkg/testing/fixture.go b/pkg/testing/fixture.go index 62ef41d3b53..3ae54837409 100644 --- a/pkg/testing/fixture.go +++ b/pkg/testing/fixture.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" + "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" "github.com/elastic/elastic-agent/internal/pkg/agent/install" @@ -1595,7 +1596,7 @@ func createTempDir(t *testing.T) string { cleanup := func() { if !t.Failed() { - if err := install.RemovePath(tempDir); err != nil { + if err := install.RemovePath(logp.L(), tempDir); err != nil { t.Errorf("could not remove temp dir '%s': %s", tempDir, err) } } else { From 43c2f4bcca18243e24daead2da74a901f0bfa42a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Thu, 2 Apr 2026 16:39:48 +0200 Subject: [PATCH 07/16] Log a warning about failing to delete a rename --- internal/pkg/agent/install/install.go | 2 +- internal/pkg/agent/install/uninstall_unix.go | 2 +- internal/pkg/agent/install/uninstall_windows.go | 8 +++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index 80030b407e8..2bd67fdb7e5 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -214,7 +214,7 @@ func setupInstallPath(log *logp.Logger, topPath string, ownership utils.FileOwne // prefix) and schedules it for deletion on reboot. If the system was not // rebooted before reinstalling, these files are still present but no // longer locked, so we can delete them now. - if err := cleanupLeftoverRenames(topPath); err != nil { + if err := cleanupLeftoverRenames(log, topPath); err != nil { log.Warnf("Failed to clean up leftover files from a previous installation in %q: %v. You may need to delete them manually.", topPath, err) } diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index 52a631ba41a..ad5a6b9ad5c 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -18,7 +18,7 @@ func isBlockingOnExe(_ error) bool { func scheduleDeleteOnReboot(_ *logp.Logger, _ error, _ string) error { return nil } -func cleanupLeftoverRenames(_ string) error { return nil } +func cleanupLeftoverRenames(_ *logp.Logger, _ string) error { return nil } func isRetryableError(_ error) bool { return false diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index fa582e5a524..30ef9f75ef5 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -93,16 +93,18 @@ func markDeleteOnReboot(path string) error { // a previous uninstall's scheduleDeleteOnReboot (files matching leftoverPrefix). // By the time a new install runs, the old process is gone and these files can // be deleted normally. -func cleanupLeftoverRenames(topPath string) error { +func cleanupLeftoverRenames(log *logp.Logger, topPath string) error { return filepath.WalkDir(topPath, func(path string, d fs.DirEntry, err error) error { if err != nil { - return nil // best-effort: skip inaccessible entries + return nil //nolint:nilerr // best-effort: skip inaccessible entries } if d.IsDir() { return nil } if strings.HasPrefix(d.Name(), leftoverPrefix) { - _ = os.Remove(path) + if err := os.Remove(path); err != nil { + log.Warnf("Failed to remove leftover file %q from a previous uninstall: %v. You may need to delete it manually.", path, err) + } } return nil }) From 34a931b623ae7d8a23a9b4bb500812e61e657ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Thu, 2 Apr 2026 16:52:00 +0200 Subject: [PATCH 08/16] Add changelog entry --- .../1775140927-refactor-uninstall.yaml | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 changelog/fragments/1775140927-refactor-uninstall.yaml diff --git a/changelog/fragments/1775140927-refactor-uninstall.yaml b/changelog/fragments/1775140927-refactor-uninstall.yaml new file mode 100644 index 00000000000..728260c8afb --- /dev/null +++ b/changelog/fragments/1775140927-refactor-uninstall.yaml @@ -0,0 +1,34 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: enhancement + +# Change summary; a 80ish characters long description of the change. +summary: Make uninstallation on Windows more robust + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +description: | + The agent executable now remains after uninstallation and is scheduled to be deleted on reboot. It can also be + removed manually afterwards. + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: elastic-agent + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +#pr: https://github.com/owner/repo/1234 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +#issue: https://github.com/owner/repo/1234 From 2ce5852185daaf9e535348a6f40e49012701f7c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Wed, 8 Apr 2026 14:04:13 +0200 Subject: [PATCH 09/16] Rename the whole root dir --- internal/pkg/agent/install/install_test.go | 31 +++--- internal/pkg/agent/install/uninstall.go | 14 +-- .../pkg/agent/install/uninstall_windows.go | 103 ++++++++++-------- .../agent/install/uninstall_windows_test.go | 43 ++++++-- 4 files changed, 116 insertions(+), 75 deletions(-) diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index 38ed5c0fa33..3b07714f79c 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -235,17 +235,20 @@ func TestSetupInstallPath(t *testing.T) { func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { tmpdir := t.TempDir() + // topPath = tmpdir/Elastic/Agent; parent = tmpdir/Elastic topPath := filepath.Join(tmpdir, "Elastic", "Agent") + parent := filepath.Dir(topPath) + require.NoError(t, os.MkdirAll(topPath, 0o755)) - // Simulate leftover renamed executable from a previous uninstall. - dataDir := filepath.Join(topPath, "data", "elastic-agent-old") - require.NoError(t, os.MkdirAll(dataDir, 0o755)) - leftoverFile := filepath.Join(dataDir, ".elastic-agent-leftover.exe") - require.NoError(t, os.WriteFile(leftoverFile, []byte("old binary"), 0o644)) + // Simulate a leftover sibling directory from a previous uninstall + // (scheduleDeleteOnReboot renames the whole install dir to a sibling). + leftoverDir := filepath.Join(parent, ".elastic-agent-leftover-12345") + require.NoError(t, os.Mkdir(leftoverDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(leftoverDir, "elastic-agent.exe"), []byte("old binary"), 0o644)) - // Also create a normal file that should NOT be deleted. - normalFile := filepath.Join(topPath, "some-config.yml") - require.NoError(t, os.WriteFile(normalFile, []byte("config"), 0o644)) + // Also create an unrelated sibling that should NOT be deleted. + unrelatedDir := filepath.Join(parent, "other-dir") + require.NoError(t, os.Mkdir(unrelatedDir, 0o755)) ownership, err := utils.CurrentFileOwner() require.NoError(t, err) @@ -257,13 +260,13 @@ func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { markerFilePath := filepath.Join(topPath, paths.MarkerFileName) require.FileExists(t, markerFilePath) - // Leftover renamed executable should be cleaned up (Windows only). + // Leftover sibling directory should be cleaned up (Windows only). if runtime.GOOS == "windows" { - _, err = os.Stat(leftoverFile) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover renamed exe should be removed") + _, err = os.Stat(leftoverDir) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover sibling directory should be removed") } - // Normal files should still be present. - _, err = os.Stat(normalFile) - assert.NoError(t, err, "normal config file should not be deleted") + // Unrelated sibling should still exist. + _, err = os.Stat(unrelatedDir) + assert.NoError(t, err, "unrelated sibling directory should not be deleted") } diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index cb9283558d0..75c51da0a54 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -296,19 +296,13 @@ func RemovePath(log *logp.Logger, path string) error { log.Debugf("RemovePath attempt %d failed after %s: %v", attempt, time.Since(start).Truncate(time.Millisecond), lastErr) if isBlockingOnExe(lastErr) { - // Rename the blocked exe in place (recognizable prefix) and - // schedule it for deletion on reboot. os.RemoveAll already - // deleted everything it could; only the exe and its ancestor - // dirs remain. + // Rename the entire install directory to a sibling with a + // recognizable prefix, then schedule it for deletion on reboot. + // After the rename, nothing remains at the original path. if err := scheduleDeleteOnReboot(log, lastErr, path); err != nil { - log.Errorf("Failed to schedule blocked file for removal: %v. You may need to manually delete %q.", err, path) + log.Errorf("Failed to schedule install directory for removal: %v. You may need to manually delete %q.", err, path) return fmt.Errorf("failed to handle blocked executable in %q: %w", path, err) } - // Try RemoveAll again — the original exe path is now free - // after the rename, so more of the tree may be removable. - if err := os.RemoveAll(path); err != nil { - log.Warnf("Some files in %q could not be removed and are scheduled for deletion on reboot or next install.", path) - } return nil } diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index 30ef9f75ef5..c76df94098e 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -21,8 +21,9 @@ import ( "github.com/elastic/elastic-agent-libs/logp" ) -// leftoverPrefix is used to rename blocked executables during uninstall. -// Files with this prefix are cleaned up on the next install. +// leftoverPrefix is used to rename the install directory during uninstall when +// a running executable prevents deletion. Sibling directories with this prefix +// are cleaned up on the next install. const leftoverPrefix = ".elastic-agent-leftover" func isBlockingOnExe(err error) bool { @@ -47,32 +48,48 @@ func isRetryableError(err error) bool { return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION } -// scheduleDeleteOnReboot renames the blocked executable in place (same -// directory, recognizable prefix) and schedules it for deletion on the next -// reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. Renaming in-place avoids -// triggering EDR software. On the next install, any leftover files matching -// the prefix are cleaned up by cleanupLeftoverRenames. -func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, _ string) error { - blockedPath, _ := getPathFromError(blockingErr) - if blockedPath == "" { - return nil - } - - dir := filepath.Dir(blockedPath) - ext := filepath.Ext(blockedPath) - renamed := filepath.Join(dir, fmt.Sprintf("%s-%d%s", leftoverPrefix, time.Now().UnixNano(), ext)) - - if err := os.Rename(blockedPath, renamed); err != nil { - return fmt.Errorf("failed to rename %q to %q: %w", blockedPath, renamed, err) +// scheduleDeleteOnReboot renames the entire rootPath directory to a sibling +// directory with a recognizable prefix, then schedules it for deletion on the +// next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. Renaming the whole +// directory (rather than moving the exe out of it) avoids triggering EDR +// software that monitors cross-directory file moves. On the next install, any +// leftover sibling directories matching the prefix are cleaned up by +// cleanupLeftoverRenames. +func scheduleDeleteOnReboot(log *logp.Logger, _ error, rootPath string) error { + parent := filepath.Dir(rootPath) + renamed := filepath.Join(parent, fmt.Sprintf("%s-%d", leftoverPrefix, time.Now().UnixNano())) + + if err := os.Rename(rootPath, renamed); err != nil { + return fmt.Errorf("failed to rename %q to %q: %w", rootPath, renamed, err) } - log.Infof("Renamed blocked executable %q to %q", blockedPath, renamed) - - if err := markDeleteOnReboot(renamed); err != nil { - log.Warnf("Failed to schedule %q for deletion on reboot: %v. The file will be cleaned up on next install.", renamed, err) - // Not fatal — the file is already renamed out of the way and will - // be cleaned up by cleanupLeftoverRenames on next install. - } else { - log.Infof("Scheduled %q for deletion on reboot", renamed) + log.Infof("Renamed install directory %q to %q", rootPath, renamed) + + // Schedule all remaining files and directories for deletion on the next + // reboot. PendingFileRenameOperations entries are processed in order, so + // scheduling files first and directories bottom-up ensures each directory + // is empty by the time the Session Manager tries to remove it. + // + // By this point os.RemoveAll has already deleted everything it could, so + // the tree will only contain the running executable and its ancestor + // directories — a very short walk. + var dirs []string + _ = filepath.WalkDir(renamed, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return nil // skip inaccessible entries + } + if d.IsDir() { + dirs = append(dirs, path) + return nil + } + if err := markDeleteOnReboot(path); err != nil { + log.Warnf("Failed to schedule %q for deletion on reboot: %v. You may need to delete it manually.", path, err) + } + return nil + }) + for i := len(dirs) - 1; i >= 0; i-- { + if err := markDeleteOnReboot(dirs[i]); err != nil { + log.Warnf("Failed to schedule directory %q for deletion on reboot: %v. You may need to delete it manually.", dirs[i], err) + } } return nil @@ -89,25 +106,25 @@ func markDeleteOnReboot(path string) error { return windows.MoveFileEx(p, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT) } -// cleanupLeftoverRenames walks topPath and removes any files left behind by -// a previous uninstall's scheduleDeleteOnReboot (files matching leftoverPrefix). -// By the time a new install runs, the old process is gone and these files can -// be deleted normally. +// cleanupLeftoverRenames removes any sibling directories left behind by a +// previous uninstall's scheduleDeleteOnReboot (directories matching +// leftoverPrefix in the parent of topPath). By the time a new install runs, +// the old process is gone and these directories can be deleted normally. func cleanupLeftoverRenames(log *logp.Logger, topPath string) error { - return filepath.WalkDir(topPath, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return nil //nolint:nilerr // best-effort: skip inaccessible entries - } - if d.IsDir() { - return nil - } - if strings.HasPrefix(d.Name(), leftoverPrefix) { - if err := os.Remove(path); err != nil { - log.Warnf("Failed to remove leftover file %q from a previous uninstall: %v. You may need to delete it manually.", path, err) + parent := filepath.Dir(topPath) + entries, err := os.ReadDir(parent) + if err != nil { + return fmt.Errorf("failed to read parent directory %q: %w", parent, err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), leftoverPrefix) { + path := filepath.Join(parent, e.Name()) + if err := os.RemoveAll(path); err != nil { + log.Warnf("Failed to remove leftover directory %q from a previous uninstall: %v. You may need to delete it manually.", path, err) } } - return nil - }) + } + return nil } func getPathFromError(blockingErr error) (string, syscall.Errno) { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index af84251c452..ae2a34c4ffe 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -121,8 +121,7 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { } // TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot renames the -// blocked executable in place with the leftover prefix and keeps it in the -// same directory. +// entire install directory to a sibling with the leftover prefix. func TestScheduleDeleteOnReboot(t *testing.T) { tmpDir := t.TempDir() destDir := filepath.Join(tmpDir, "target") @@ -134,12 +133,12 @@ func TestScheduleDeleteOnReboot(t *testing.T) { err := scheduleDeleteOnReboot(logp.L(), blockingErr, destDir) require.NoError(t, err) - // The original path should be gone (renamed). - _, err = os.Stat(exePath) - assert.ErrorIs(t, err, fs.ErrNotExist, "original exe path should no longer exist") + // The original install directory should be gone (renamed to a sibling). + _, err = os.Stat(destDir) + assert.ErrorIs(t, err, fs.ErrNotExist, "original install directory should no longer exist") - // A file with the leftover prefix should exist in the same directory. - entries, err := os.ReadDir(destDir) + // A sibling directory with the leftover prefix should exist in the parent. + entries, err := os.ReadDir(tmpDir) require.NoError(t, err) var found bool @@ -149,7 +148,35 @@ func TestScheduleDeleteOnReboot(t *testing.T) { break } } - assert.True(t, found, "renamed file with prefix %q should exist in %s", leftoverPrefix, destDir) + assert.True(t, found, "sibling directory with prefix %q should exist in %s", leftoverPrefix, tmpDir) +} + +// TestCleanupLeftoverRenames verifies that cleanupLeftoverRenames removes +// leftover sibling directories from a previous uninstall. +func TestCleanupLeftoverRenames(t *testing.T) { + tmpDir := t.TempDir() + installDir := filepath.Join(tmpDir, "Agent") + require.NoError(t, os.Mkdir(installDir, 0o755)) + + // Create a leftover sibling directory from a previous uninstall. + leftover := filepath.Join(tmpDir, leftoverPrefix+"-12345678") + require.NoError(t, os.Mkdir(leftover, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(leftover, "elastic-agent.exe"), []byte("fake"), 0o644)) + + // Create an unrelated sibling that should NOT be removed. + unrelated := filepath.Join(tmpDir, "other-dir") + require.NoError(t, os.Mkdir(unrelated, 0o755)) + + err := cleanupLeftoverRenames(logp.L(), installDir) + require.NoError(t, err) + + // The leftover sibling should be gone. + _, err = os.Stat(leftover) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover directory should be removed") + + // The unrelated sibling should still exist. + _, err = os.Stat(unrelated) + assert.NoError(t, err, "unrelated sibling directory should be preserved") } // TestRemoveAllDeletesEverythingExceptBlockingExe verifies that os.RemoveAll From 17c06a4bf947bf8e9d07cb3d083f039f416fd61f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Wed, 8 Apr 2026 14:29:41 +0200 Subject: [PATCH 10/16] make linter happy --- internal/pkg/agent/install/uninstall_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index c76df94098e..0a83040c8ff 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -75,7 +75,7 @@ func scheduleDeleteOnReboot(log *logp.Logger, _ error, rootPath string) error { var dirs []string _ = filepath.WalkDir(renamed, func(path string, d fs.DirEntry, err error) error { if err != nil { - return nil // skip inaccessible entries + return filepath.SkipDir // skip inaccessible entries } if d.IsDir() { dirs = append(dirs, path) From c73ade7189cf7b576dde453115e7983d5ce8ea47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Wed, 8 Apr 2026 18:27:51 +0200 Subject: [PATCH 11/16] Rename the versioned dir instead --- internal/pkg/agent/install/install_test.go | 23 ++++--- internal/pkg/agent/install/uninstall.go | 10 +-- .../pkg/agent/install/uninstall_windows.go | 65 ++++++++++--------- .../agent/install/uninstall_windows_test.go | 45 +++++++------ 4 files changed, 77 insertions(+), 66 deletions(-) diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index 3b07714f79c..c277dbf8834 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -235,19 +235,18 @@ func TestSetupInstallPath(t *testing.T) { func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { tmpdir := t.TempDir() - // topPath = tmpdir/Elastic/Agent; parent = tmpdir/Elastic topPath := filepath.Join(tmpdir, "Elastic", "Agent") - parent := filepath.Dir(topPath) - require.NoError(t, os.MkdirAll(topPath, 0o755)) + dataDir := filepath.Join(topPath, "data") + require.NoError(t, os.MkdirAll(dataDir, 0o755)) - // Simulate a leftover sibling directory from a previous uninstall - // (scheduleDeleteOnReboot renames the whole install dir to a sibling). - leftoverDir := filepath.Join(parent, ".elastic-agent-leftover-12345") + // Simulate a leftover directory inside data from a previous uninstall + // (scheduleDeleteOnReboot renames the versioned dir to a sibling in data). + leftoverDir := filepath.Join(dataDir, ".elastic-agent-leftover-12345") require.NoError(t, os.Mkdir(leftoverDir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(leftoverDir, "elastic-agent.exe"), []byte("old binary"), 0o644)) - // Also create an unrelated sibling that should NOT be deleted. - unrelatedDir := filepath.Join(parent, "other-dir") + // Create an unrelated directory inside data that should NOT be deleted. + unrelatedDir := filepath.Join(dataDir, "elastic-agent-8.15.0") require.NoError(t, os.Mkdir(unrelatedDir, 0o755)) ownership, err := utils.CurrentFileOwner() @@ -260,13 +259,13 @@ func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { markerFilePath := filepath.Join(topPath, paths.MarkerFileName) require.FileExists(t, markerFilePath) - // Leftover sibling directory should be cleaned up (Windows only). + // Leftover directory should be cleaned up (Windows only). if runtime.GOOS == "windows" { _, err = os.Stat(leftoverDir) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover sibling directory should be removed") + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover directory should be removed") } - // Unrelated sibling should still exist. + // Unrelated directory should still exist. _, err = os.Stat(unrelatedDir) - assert.NoError(t, err, "unrelated sibling directory should not be deleted") + assert.NoError(t, err, "unrelated directory should not be deleted") } diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index 75c51da0a54..1b587d4fe76 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -296,11 +296,13 @@ func RemovePath(log *logp.Logger, path string) error { log.Debugf("RemovePath attempt %d failed after %s: %v", attempt, time.Since(start).Truncate(time.Millisecond), lastErr) if isBlockingOnExe(lastErr) { - // Rename the entire install directory to a sibling with a - // recognizable prefix, then schedule it for deletion on reboot. - // After the rename, nothing remains at the original path. + // Rename the versioned directory (direct parent of the blocked + // exe) and schedule it for deletion on reboot. The first + // os.RemoveAll already deleted everything else it could; the + // renamed directory still holds the running exe so another + // RemoveAll would fail the same way. if err := scheduleDeleteOnReboot(log, lastErr, path); err != nil { - log.Errorf("Failed to schedule install directory for removal: %v. You may need to manually delete %q.", err, path) + log.Errorf("Failed to schedule blocked executable for removal: %v. You may need to manually delete %q.", err, path) return fmt.Errorf("failed to handle blocked executable in %q: %w", path, err) } return nil diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index 0a83040c8ff..a7bc6df1bd0 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -48,21 +48,28 @@ func isRetryableError(err error) bool { return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION } -// scheduleDeleteOnReboot renames the entire rootPath directory to a sibling -// directory with a recognizable prefix, then schedules it for deletion on the -// next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. Renaming the whole -// directory (rather than moving the exe out of it) avoids triggering EDR -// software that monitors cross-directory file moves. On the next install, any -// leftover sibling directories matching the prefix are cleaned up by -// cleanupLeftoverRenames. -func scheduleDeleteOnReboot(log *logp.Logger, _ error, rootPath string) error { - parent := filepath.Dir(rootPath) +// scheduleDeleteOnReboot renames the versioned directory containing the blocked +// executable to a sibling with a recognizable prefix, then schedules it for +// deletion on the next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. +// Renaming within the same parent directory avoids needing write permission to +// the parent of the install root and avoids triggering EDR software that +// monitors cross-directory moves. On the next install, any leftover directories +// matching the prefix are cleaned up by cleanupLeftoverRenames. +func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, _ string) error { + blockedPath, _ := getPathFromError(blockingErr) + if blockedPath == "" { + return fmt.Errorf("could not determine blocked path from error: %w", blockingErr) + } + + // The versioned directory is the direct parent of the blocked exe. + versionedDir := filepath.Dir(blockedPath) + parent := filepath.Dir(versionedDir) renamed := filepath.Join(parent, fmt.Sprintf("%s-%d", leftoverPrefix, time.Now().UnixNano())) - if err := os.Rename(rootPath, renamed); err != nil { - return fmt.Errorf("failed to rename %q to %q: %w", rootPath, renamed, err) + if err := os.Rename(versionedDir, renamed); err != nil { + return fmt.Errorf("failed to rename %q to %q: %w", versionedDir, renamed, err) } - log.Infof("Renamed install directory %q to %q", rootPath, renamed) + log.Infof("Renamed %q to %q", versionedDir, renamed) // Schedule all remaining files and directories for deletion on the next // reboot. PendingFileRenameOperations entries are processed in order, so @@ -75,7 +82,7 @@ func scheduleDeleteOnReboot(log *logp.Logger, _ error, rootPath string) error { var dirs []string _ = filepath.WalkDir(renamed, func(path string, d fs.DirEntry, err error) error { if err != nil { - return filepath.SkipDir // skip inaccessible entries + return filepath.SkipDir } if d.IsDir() { dirs = append(dirs, path) @@ -106,25 +113,23 @@ func markDeleteOnReboot(path string) error { return windows.MoveFileEx(p, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT) } -// cleanupLeftoverRenames removes any sibling directories left behind by a -// previous uninstall's scheduleDeleteOnReboot (directories matching -// leftoverPrefix in the parent of topPath). By the time a new install runs, -// the old process is gone and these directories can be deleted normally. +// cleanupLeftoverRenames walks topPath and removes any directories left behind +// by a previous uninstall's scheduleDeleteOnReboot (directories matching +// leftoverPrefix anywhere under topPath). By the time a new install runs, the +// old process is gone and these directories can be deleted normally. func cleanupLeftoverRenames(log *logp.Logger, topPath string) error { - parent := filepath.Dir(topPath) - entries, err := os.ReadDir(parent) - if err != nil { - return fmt.Errorf("failed to read parent directory %q: %w", parent, err) - } - for _, e := range entries { - if strings.HasPrefix(e.Name(), leftoverPrefix) { - path := filepath.Join(parent, e.Name()) - if err := os.RemoveAll(path); err != nil { - log.Warnf("Failed to remove leftover directory %q from a previous uninstall: %v. You may need to delete it manually.", path, err) - } + return filepath.WalkDir(topPath, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return nil //nolint:nilerr // best-effort: skip inaccessible entries } - } - return nil + if !d.IsDir() || !strings.HasPrefix(d.Name(), leftoverPrefix) { + return nil + } + if err := os.RemoveAll(path); err != nil { + log.Warnf("Failed to remove leftover directory %q from a previous uninstall: %v. You may need to delete it manually.", path, err) + } + return filepath.SkipDir + }) } func getPathFromError(blockingErr error) (string, syscall.Errno) { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index ae2a34c4ffe..17343648de3 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -121,24 +121,27 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { } // TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot renames the -// entire install directory to a sibling with the leftover prefix. +// versioned directory (direct parent of the blocked exe) to a sibling with the +// leftover prefix, leaving the install root intact. func TestScheduleDeleteOnReboot(t *testing.T) { tmpDir := t.TempDir() - destDir := filepath.Join(tmpDir, "target") - require.NoError(t, os.Mkdir(destDir, 0o755)) + // Simulate the real layout: /data//elastic-agent.exe + dataDir := filepath.Join(tmpDir, "target", "data") + versionedDir := filepath.Join(dataDir, "elastic-agent-8.15.0") + require.NoError(t, os.MkdirAll(versionedDir, 0o755)) - _, exePath := startBlockingExe(t, destDir) + _, exePath := startBlockingExe(t, versionedDir) blockingErr := makeBlockingError(exePath) - err := scheduleDeleteOnReboot(logp.L(), blockingErr, destDir) + err := scheduleDeleteOnReboot(logp.L(), blockingErr, filepath.Join(tmpDir, "target")) require.NoError(t, err) - // The original install directory should be gone (renamed to a sibling). - _, err = os.Stat(destDir) - assert.ErrorIs(t, err, fs.ErrNotExist, "original install directory should no longer exist") + // The versioned directory should be gone (renamed to a sibling in dataDir). + _, err = os.Stat(versionedDir) + assert.ErrorIs(t, err, fs.ErrNotExist, "versioned directory should no longer exist") - // A sibling directory with the leftover prefix should exist in the parent. - entries, err := os.ReadDir(tmpDir) + // A sibling directory with the leftover prefix should exist in dataDir. + entries, err := os.ReadDir(dataDir) require.NoError(t, err) var found bool @@ -148,35 +151,37 @@ func TestScheduleDeleteOnReboot(t *testing.T) { break } } - assert.True(t, found, "sibling directory with prefix %q should exist in %s", leftoverPrefix, tmpDir) + assert.True(t, found, "sibling directory with prefix %q should exist in %s", leftoverPrefix, dataDir) } // TestCleanupLeftoverRenames verifies that cleanupLeftoverRenames removes -// leftover sibling directories from a previous uninstall. +// leftover directories anywhere under topPath from a previous uninstall. func TestCleanupLeftoverRenames(t *testing.T) { tmpDir := t.TempDir() installDir := filepath.Join(tmpDir, "Agent") - require.NoError(t, os.Mkdir(installDir, 0o755)) + dataDir := filepath.Join(installDir, "data") + require.NoError(t, os.MkdirAll(dataDir, 0o755)) - // Create a leftover sibling directory from a previous uninstall. - leftover := filepath.Join(tmpDir, leftoverPrefix+"-12345678") + // Create a leftover directory inside the data dir (sibling of the + // versioned dir, as scheduleDeleteOnReboot produces). + leftover := filepath.Join(dataDir, leftoverPrefix+"-12345678") require.NoError(t, os.Mkdir(leftover, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(leftover, "elastic-agent.exe"), []byte("fake"), 0o644)) - // Create an unrelated sibling that should NOT be removed. - unrelated := filepath.Join(tmpDir, "other-dir") + // Create an unrelated directory that should NOT be removed. + unrelated := filepath.Join(dataDir, "elastic-agent-8.15.0") require.NoError(t, os.Mkdir(unrelated, 0o755)) err := cleanupLeftoverRenames(logp.L(), installDir) require.NoError(t, err) - // The leftover sibling should be gone. + // The leftover directory should be gone. _, err = os.Stat(leftover) assert.ErrorIs(t, err, fs.ErrNotExist, "leftover directory should be removed") - // The unrelated sibling should still exist. + // The unrelated directory should still exist. _, err = os.Stat(unrelated) - assert.NoError(t, err, "unrelated sibling directory should be preserved") + assert.NoError(t, err, "unrelated directory should not be deleted") } // TestRemoveAllDeletesEverythingExceptBlockingExe verifies that os.RemoveAll From 258ce7e459ab0eb91d06c52b05f7a6b211730052 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Thu, 9 Apr 2026 14:54:07 +0200 Subject: [PATCH 12/16] Always use the versioned path --- .../pkg/agent/install/uninstall_windows.go | 29 +++++++++++++++++-- .../agent/install/uninstall_windows_test.go | 17 ++++++----- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index a7bc6df1bd0..83a8ebbcef7 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -55,14 +55,20 @@ func isRetryableError(err error) bool { // the parent of the install root and avoids triggering EDR software that // monitors cross-directory moves. On the next install, any leftover directories // matching the prefix are cleaned up by cleanupLeftoverRenames. -func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, _ string) error { +func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, rootPath string) error { blockedPath, _ := getPathFromError(blockingErr) if blockedPath == "" { return fmt.Errorf("could not determine blocked path from error: %w", blockingErr) } - // The versioned directory is the direct parent of the blocked exe. - versionedDir := filepath.Dir(blockedPath) + // The blocked exe may be anywhere within the versioned directory (e.g. + // directly inside it or nested under components\). Find the ancestor that + // sits two levels below rootPath (rootPath\data\) so we rename + // at the right level regardless of the exe's depth. + versionedDir, err := versionedDirFromBlocked(rootPath, blockedPath) + if err != nil { + return err + } parent := filepath.Dir(versionedDir) renamed := filepath.Join(parent, fmt.Sprintf("%s-%d", leftoverPrefix, time.Now().UnixNano())) @@ -102,6 +108,23 @@ func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, _ string) error return nil } +// versionedDirFromBlocked returns the ancestor of blockedPath that is a direct +// child of rootPath's second level (rootPath\data\). The blocked +// executable may be nested arbitrarily deep within the versioned directory, so +// we cannot rely on filepath.Dir alone. +func versionedDirFromBlocked(rootPath, blockedPath string) (string, error) { + rel, err := filepath.Rel(rootPath, blockedPath) + if err != nil || strings.HasPrefix(rel, "..") { + return "", fmt.Errorf("blocked path %q is not within install root %q", blockedPath, rootPath) + } + // Split into at most 3 parts: e.g. ["data", "elastic-agent-X.Y.Z", "rest"] + parts := strings.SplitN(filepath.ToSlash(rel), "/", 3) + if len(parts) < 2 { + return "", fmt.Errorf("blocked path %q is too shallow within install root %q", blockedPath, rootPath) + } + return filepath.Join(rootPath, parts[0], parts[1]), nil +} + // markDeleteOnReboot schedules a file or directory for deletion on the next // Windows reboot. This only writes a registry entry // (PendingFileRenameOperations) and does not touch the file itself. diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index 17343648de3..e2a66dccad9 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -121,19 +121,20 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { } // TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot renames the -// versioned directory (direct parent of the blocked exe) to a sibling with the -// leftover prefix, leaving the install root intact. +// versioned directory to a sibling with the leftover prefix, regardless of how +// deeply nested the blocked executable is within that directory. func TestScheduleDeleteOnReboot(t *testing.T) { - tmpDir := t.TempDir() - // Simulate the real layout: /data//elastic-agent.exe - dataDir := filepath.Join(tmpDir, "target", "data") + rootPath := filepath.Join(t.TempDir(), "target") + dataDir := filepath.Join(rootPath, "data") versionedDir := filepath.Join(dataDir, "elastic-agent-8.15.0") - require.NoError(t, os.MkdirAll(versionedDir, 0o755)) + // Simulate the real layout: the exe lives inside a components subdirectory. + componentsDir := filepath.Join(versionedDir, "components") + require.NoError(t, os.MkdirAll(componentsDir, 0o755)) - _, exePath := startBlockingExe(t, versionedDir) + _, exePath := startBlockingExe(t, componentsDir) blockingErr := makeBlockingError(exePath) - err := scheduleDeleteOnReboot(logp.L(), blockingErr, filepath.Join(tmpDir, "target")) + err := scheduleDeleteOnReboot(logp.L(), blockingErr, rootPath) require.NoError(t, err) // The versioned directory should be gone (renamed to a sibling in dataDir). From c19d0cdc4061305991608e4f401c7281e5f15edd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Thu, 9 Apr 2026 17:11:52 +0200 Subject: [PATCH 13/16] Fix unit test --- internal/pkg/agent/install/uninstall_windows_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index e2a66dccad9..4413632ea0a 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -60,12 +60,13 @@ func startBlockingExe(t *testing.T, destDir string) (*exec.Cmd, string) { // a directory containing a running executable (or schedules leftovers for // reboot deletion and returns nil). func TestRemovePath(t *testing.T) { - destDir := filepath.Join(t.TempDir(), "target") - require.NoError(t, os.Mkdir(destDir, 0o755)) + rootPath := filepath.Join(t.TempDir(), "target") + versionedDir := filepath.Join(rootPath, "data", "elastic-agent-8.15.0") + require.NoError(t, os.MkdirAll(versionedDir, 0o755)) - startBlockingExe(t, destDir) + startBlockingExe(t, versionedDir) - err := RemovePath(logp.L(), destDir) + err := RemovePath(logp.L(), rootPath) assert.NoError(t, err) } From 87e77220763ef72fac51a02d395e839edabc021c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Fri, 10 Apr 2026 11:53:46 +0200 Subject: [PATCH 14/16] Switch back to moving the file --- internal/pkg/agent/install/install_test.go | 30 +++-- internal/pkg/agent/install/uninstall.go | 14 ++- .../pkg/agent/install/uninstall_windows.go | 116 ++++++------------ .../agent/install/uninstall_windows_test.go | 59 ++++----- 4 files changed, 86 insertions(+), 133 deletions(-) diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index c277dbf8834..38a19754c1f 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -236,18 +236,16 @@ func TestSetupInstallPath(t *testing.T) { func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { tmpdir := t.TempDir() topPath := filepath.Join(tmpdir, "Elastic", "Agent") - dataDir := filepath.Join(topPath, "data") - require.NoError(t, os.MkdirAll(dataDir, 0o755)) + require.NoError(t, os.MkdirAll(topPath, 0o755)) - // Simulate a leftover directory inside data from a previous uninstall - // (scheduleDeleteOnReboot renames the versioned dir to a sibling in data). - leftoverDir := filepath.Join(dataDir, ".elastic-agent-leftover-12345") - require.NoError(t, os.Mkdir(leftoverDir, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(leftoverDir, "elastic-agent.exe"), []byte("old binary"), 0o644)) + // Simulate a leftover file at the install root from a previous uninstall + // (scheduleDeleteOnReboot renames the blocked exe to rootPath/-.exe). + leftoverFile := filepath.Join(topPath, ".elastic-agent-leftover-12345.exe") + require.NoError(t, os.WriteFile(leftoverFile, []byte("old binary"), 0o644)) - // Create an unrelated directory inside data that should NOT be deleted. - unrelatedDir := filepath.Join(dataDir, "elastic-agent-8.15.0") - require.NoError(t, os.Mkdir(unrelatedDir, 0o755)) + // Create an unrelated file that should NOT be deleted. + unrelatedFile := filepath.Join(topPath, "elastic-agent.yml") + require.NoError(t, os.WriteFile(unrelatedFile, []byte("config"), 0o644)) ownership, err := utils.CurrentFileOwner() require.NoError(t, err) @@ -259,13 +257,13 @@ func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { markerFilePath := filepath.Join(topPath, paths.MarkerFileName) require.FileExists(t, markerFilePath) - // Leftover directory should be cleaned up (Windows only). + // Leftover file should be cleaned up (Windows only). if runtime.GOOS == "windows" { - _, err = os.Stat(leftoverDir) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover directory should be removed") + _, err = os.Stat(leftoverFile) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover file should be removed") } - // Unrelated directory should still exist. - _, err = os.Stat(unrelatedDir) - assert.NoError(t, err, "unrelated directory should not be deleted") + // Unrelated file should still exist. + _, err = os.Stat(unrelatedFile) + assert.NoError(t, err, "unrelated file should not be deleted") } diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index 1b587d4fe76..cef58a205ee 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -296,15 +296,19 @@ func RemovePath(log *logp.Logger, path string) error { log.Debugf("RemovePath attempt %d failed after %s: %v", attempt, time.Since(start).Truncate(time.Millisecond), lastErr) if isBlockingOnExe(lastErr) { - // Rename the versioned directory (direct parent of the blocked - // exe) and schedule it for deletion on reboot. The first - // os.RemoveAll already deleted everything else it could; the - // renamed directory still holds the running exe so another - // RemoveAll would fail the same way. + // Rename the blocked exe to the install root with a recognizable + // prefix and schedule it for reboot deletion. Then retry + // RemoveAll — with the exe out of its original location the + // directory skeleton should now be removable. The renamed file + // at the root will still block full removal of the root itself, + // which is acceptable. if err := scheduleDeleteOnReboot(log, lastErr, path); err != nil { log.Errorf("Failed to schedule blocked executable for removal: %v. You may need to manually delete %q.", err, path) return fmt.Errorf("failed to handle blocked executable in %q: %w", path, err) } + if err := os.RemoveAll(path); err != nil { + log.Warnf("Some files in %q could not be removed and are scheduled for deletion on reboot or next install.", path) + } return nil } diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index 83a8ebbcef7..c9eb6900185 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -21,9 +21,8 @@ import ( "github.com/elastic/elastic-agent-libs/logp" ) -// leftoverPrefix is used to rename the install directory during uninstall when -// a running executable prevents deletion. Sibling directories with this prefix -// are cleaned up on the next install. +// leftoverPrefix is used to rename blocked executables during uninstall. +// Files with this prefix placed at the install root are cleaned up on the next install. const leftoverPrefix = ".elastic-agent-leftover" func isBlockingOnExe(err error) bool { @@ -48,83 +47,36 @@ func isRetryableError(err error) bool { return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION } -// scheduleDeleteOnReboot renames the versioned directory containing the blocked -// executable to a sibling with a recognizable prefix, then schedules it for -// deletion on the next reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. -// Renaming within the same parent directory avoids needing write permission to -// the parent of the install root and avoids triggering EDR software that -// monitors cross-directory moves. On the next install, any leftover directories -// matching the prefix are cleaned up by cleanupLeftoverRenames. +// scheduleDeleteOnReboot renames the blocked executable to the install root +// with a recognizable prefix, then schedules it for deletion on the next +// reboot via MoveFileEx/MOVEFILE_DELAY_UNTIL_REBOOT. Renaming to the install +// root (rather than to an external temp dir) keeps the file on the same volume +// and within the install tree, avoiding cross-directory move heuristics in EDR +// software. On the next install, leftover files matching the prefix are cleaned +// up by cleanupLeftoverRenames. func scheduleDeleteOnReboot(log *logp.Logger, blockingErr error, rootPath string) error { blockedPath, _ := getPathFromError(blockingErr) if blockedPath == "" { return fmt.Errorf("could not determine blocked path from error: %w", blockingErr) } - // The blocked exe may be anywhere within the versioned directory (e.g. - // directly inside it or nested under components\). Find the ancestor that - // sits two levels below rootPath (rootPath\data\) so we rename - // at the right level regardless of the exe's depth. - versionedDir, err := versionedDirFromBlocked(rootPath, blockedPath) - if err != nil { - return err - } - parent := filepath.Dir(versionedDir) - renamed := filepath.Join(parent, fmt.Sprintf("%s-%d", leftoverPrefix, time.Now().UnixNano())) + ext := filepath.Ext(blockedPath) + renamed := filepath.Join(rootPath, fmt.Sprintf("%s-%d%s", leftoverPrefix, time.Now().UnixNano(), ext)) - if err := os.Rename(versionedDir, renamed); err != nil { - return fmt.Errorf("failed to rename %q to %q: %w", versionedDir, renamed, err) + if err := os.Rename(blockedPath, renamed); err != nil { + return fmt.Errorf("failed to rename %q to %q: %w", blockedPath, renamed, err) } - log.Infof("Renamed %q to %q", versionedDir, renamed) - - // Schedule all remaining files and directories for deletion on the next - // reboot. PendingFileRenameOperations entries are processed in order, so - // scheduling files first and directories bottom-up ensures each directory - // is empty by the time the Session Manager tries to remove it. - // - // By this point os.RemoveAll has already deleted everything it could, so - // the tree will only contain the running executable and its ancestor - // directories — a very short walk. - var dirs []string - _ = filepath.WalkDir(renamed, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return filepath.SkipDir - } - if d.IsDir() { - dirs = append(dirs, path) - return nil - } - if err := markDeleteOnReboot(path); err != nil { - log.Warnf("Failed to schedule %q for deletion on reboot: %v. You may need to delete it manually.", path, err) - } - return nil - }) - for i := len(dirs) - 1; i >= 0; i-- { - if err := markDeleteOnReboot(dirs[i]); err != nil { - log.Warnf("Failed to schedule directory %q for deletion on reboot: %v. You may need to delete it manually.", dirs[i], err) - } + log.Infof("Renamed blocked executable %q to %q", blockedPath, renamed) + + if err := markDeleteOnReboot(renamed); err != nil { + log.Warnf("Failed to schedule %q for deletion on reboot: %v. You may need to delete it manually.", renamed, err) + } else { + log.Infof("Scheduled %q for deletion on reboot", renamed) } return nil } -// versionedDirFromBlocked returns the ancestor of blockedPath that is a direct -// child of rootPath's second level (rootPath\data\). The blocked -// executable may be nested arbitrarily deep within the versioned directory, so -// we cannot rely on filepath.Dir alone. -func versionedDirFromBlocked(rootPath, blockedPath string) (string, error) { - rel, err := filepath.Rel(rootPath, blockedPath) - if err != nil || strings.HasPrefix(rel, "..") { - return "", fmt.Errorf("blocked path %q is not within install root %q", blockedPath, rootPath) - } - // Split into at most 3 parts: e.g. ["data", "elastic-agent-X.Y.Z", "rest"] - parts := strings.SplitN(filepath.ToSlash(rel), "/", 3) - if len(parts) < 2 { - return "", fmt.Errorf("blocked path %q is too shallow within install root %q", blockedPath, rootPath) - } - return filepath.Join(rootPath, parts[0], parts[1]), nil -} - // markDeleteOnReboot schedules a file or directory for deletion on the next // Windows reboot. This only writes a registry entry // (PendingFileRenameOperations) and does not touch the file itself. @@ -136,23 +88,27 @@ func markDeleteOnReboot(path string) error { return windows.MoveFileEx(p, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT) } -// cleanupLeftoverRenames walks topPath and removes any directories left behind -// by a previous uninstall's scheduleDeleteOnReboot (directories matching -// leftoverPrefix anywhere under topPath). By the time a new install runs, the -// old process is gone and these directories can be deleted normally. +// cleanupLeftoverRenames removes any files at the top level of topPath left +// behind by a previous uninstall's scheduleDeleteOnReboot (files matching +// leftoverPrefix). By the time a new install runs, the old process is gone +// and these files can be deleted normally. func cleanupLeftoverRenames(log *logp.Logger, topPath string) error { - return filepath.WalkDir(topPath, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return nil //nolint:nilerr // best-effort: skip inaccessible entries - } - if !d.IsDir() || !strings.HasPrefix(d.Name(), leftoverPrefix) { + entries, err := os.ReadDir(topPath) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { return nil } - if err := os.RemoveAll(path); err != nil { - log.Warnf("Failed to remove leftover directory %q from a previous uninstall: %v. You may need to delete it manually.", path, err) + return fmt.Errorf("failed to read %q: %w", topPath, err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), leftoverPrefix) { + path := filepath.Join(topPath, e.Name()) + if err := os.Remove(path); err != nil { + log.Warnf("Failed to remove leftover file %q from a previous uninstall: %v. You may need to delete it manually.", path, err) + } } - return filepath.SkipDir - }) + } + return nil } func getPathFromError(blockingErr error) (string, syscall.Errno) { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index 4413632ea0a..bd596c56249 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/internal/pkg/agent/install/uninstall_windows_test.go @@ -61,10 +61,11 @@ func startBlockingExe(t *testing.T, destDir string) (*exec.Cmd, string) { // reboot deletion and returns nil). func TestRemovePath(t *testing.T) { rootPath := filepath.Join(t.TempDir(), "target") - versionedDir := filepath.Join(rootPath, "data", "elastic-agent-8.15.0") - require.NoError(t, os.MkdirAll(versionedDir, 0o755)) + // Simulate the real layout: exe nested inside the versioned/components dirs. + componentsDir := filepath.Join(rootPath, "data", "elastic-agent-8.15.0", "components") + require.NoError(t, os.MkdirAll(componentsDir, 0o755)) - startBlockingExe(t, versionedDir) + startBlockingExe(t, componentsDir) err := RemovePath(logp.L(), rootPath) assert.NoError(t, err) @@ -122,14 +123,12 @@ func TestRemoveAllSucceedsAfterRename(t *testing.T) { } // TestScheduleDeleteOnReboot verifies that scheduleDeleteOnReboot renames the -// versioned directory to a sibling with the leftover prefix, regardless of how -// deeply nested the blocked executable is within that directory. +// blocked executable to the install root with the leftover prefix, regardless +// of how deeply nested the exe is within the install tree. func TestScheduleDeleteOnReboot(t *testing.T) { rootPath := filepath.Join(t.TempDir(), "target") - dataDir := filepath.Join(rootPath, "data") - versionedDir := filepath.Join(dataDir, "elastic-agent-8.15.0") - // Simulate the real layout: the exe lives inside a components subdirectory. - componentsDir := filepath.Join(versionedDir, "components") + // Simulate the real layout: exe nested inside components. + componentsDir := filepath.Join(rootPath, "data", "elastic-agent-8.15.0", "components") require.NoError(t, os.MkdirAll(componentsDir, 0o755)) _, exePath := startBlockingExe(t, componentsDir) @@ -138,12 +137,12 @@ func TestScheduleDeleteOnReboot(t *testing.T) { err := scheduleDeleteOnReboot(logp.L(), blockingErr, rootPath) require.NoError(t, err) - // The versioned directory should be gone (renamed to a sibling in dataDir). - _, err = os.Stat(versionedDir) - assert.ErrorIs(t, err, fs.ErrNotExist, "versioned directory should no longer exist") + // The original exe path should be gone. + _, err = os.Stat(exePath) + assert.ErrorIs(t, err, fs.ErrNotExist, "original exe path should no longer exist") - // A sibling directory with the leftover prefix should exist in dataDir. - entries, err := os.ReadDir(dataDir) + // A leftover file with the prefix should exist at the install root. + entries, err := os.ReadDir(rootPath) require.NoError(t, err) var found bool @@ -153,37 +152,33 @@ func TestScheduleDeleteOnReboot(t *testing.T) { break } } - assert.True(t, found, "sibling directory with prefix %q should exist in %s", leftoverPrefix, dataDir) + assert.True(t, found, "leftover file with prefix %q should exist in %s", leftoverPrefix, rootPath) } // TestCleanupLeftoverRenames verifies that cleanupLeftoverRenames removes -// leftover directories anywhere under topPath from a previous uninstall. +// leftover files at the top level of topPath from a previous uninstall. func TestCleanupLeftoverRenames(t *testing.T) { - tmpDir := t.TempDir() - installDir := filepath.Join(tmpDir, "Agent") - dataDir := filepath.Join(installDir, "data") - require.NoError(t, os.MkdirAll(dataDir, 0o755)) + installDir := filepath.Join(t.TempDir(), "Agent") + require.NoError(t, os.Mkdir(installDir, 0o755)) - // Create a leftover directory inside the data dir (sibling of the - // versioned dir, as scheduleDeleteOnReboot produces). - leftover := filepath.Join(dataDir, leftoverPrefix+"-12345678") - require.NoError(t, os.Mkdir(leftover, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(leftover, "elastic-agent.exe"), []byte("fake"), 0o644)) + // Create a leftover file at the install root (as scheduleDeleteOnReboot produces). + leftover := filepath.Join(installDir, leftoverPrefix+"-12345678.exe") + require.NoError(t, os.WriteFile(leftover, []byte("fake"), 0o644)) - // Create an unrelated directory that should NOT be removed. - unrelated := filepath.Join(dataDir, "elastic-agent-8.15.0") - require.NoError(t, os.Mkdir(unrelated, 0o755)) + // Create an unrelated file that should NOT be removed. + unrelated := filepath.Join(installDir, "elastic-agent.yml") + require.NoError(t, os.WriteFile(unrelated, []byte("config"), 0o644)) err := cleanupLeftoverRenames(logp.L(), installDir) require.NoError(t, err) - // The leftover directory should be gone. + // The leftover file should be gone. _, err = os.Stat(leftover) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover directory should be removed") + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover file should be removed") - // The unrelated directory should still exist. + // The unrelated file should still exist. _, err = os.Stat(unrelated) - assert.NoError(t, err, "unrelated directory should not be deleted") + assert.NoError(t, err, "unrelated file should not be deleted") } // TestRemoveAllDeletesEverythingExceptBlockingExe verifies that os.RemoveAll From 6ddae7ee7f59ce58013bacd91dcdc4b764689c38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Fri, 10 Apr 2026 16:38:43 +0200 Subject: [PATCH 15/16] Minor test fix --- internal/pkg/agent/install/install_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index 38a19754c1f..9e016c5dc02 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -9,7 +9,6 @@ import ( "io/fs" "os" "path/filepath" - "runtime" "testing" "github.com/jaypipes/ghw" @@ -257,11 +256,8 @@ func TestSetupInstallPathCleansUpLeftoverRenames(t *testing.T) { markerFilePath := filepath.Join(topPath, paths.MarkerFileName) require.FileExists(t, markerFilePath) - // Leftover file should be cleaned up (Windows only). - if runtime.GOOS == "windows" { - _, err = os.Stat(leftoverFile) - assert.ErrorIs(t, err, fs.ErrNotExist, "leftover file should be removed") - } + _, err = os.Stat(leftoverFile) + assert.ErrorIs(t, err, fs.ErrNotExist, "leftover file should be removed") // Unrelated file should still exist. _, err = os.Stat(unrelatedFile) From 4e609912ca8f585799920a1af99271430119d3e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Fri, 10 Apr 2026 20:39:32 +0200 Subject: [PATCH 16/16] Make some functions universal --- internal/pkg/agent/install/uninstall.go | 27 ++++++++++++++++++ internal/pkg/agent/install/uninstall_unix.go | 2 -- .../pkg/agent/install/uninstall_windows.go | 28 ------------------- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index cef58a205ee..78f3725128d 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -273,6 +273,33 @@ func checkForUnprivilegedVault(ctx context.Context, opts ...vault.OptionFunc) (b return false, nil } +// leftoverPrefix is used to rename blocked executables during uninstall. +// Files with this prefix placed at the install root are cleaned up on the next install. +const leftoverPrefix = ".elastic-agent-leftover" + +// cleanupLeftoverRenames removes any files at the top level of topPath left +// behind by a previous uninstall's scheduleDeleteOnReboot (files matching +// leftoverPrefix). By the time a new install runs, the old process is gone +// and these files can be deleted normally. +func cleanupLeftoverRenames(log *logp.Logger, topPath string) error { + entries, err := os.ReadDir(topPath) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return fmt.Errorf("failed to read %q: %w", topPath, err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), leftoverPrefix) { + path := filepath.Join(topPath, e.Name()) + if err := os.Remove(path); err != nil { + log.Warnf("Failed to remove leftover file %q from a previous uninstall: %v. You may need to delete it manually.", path, err) + } + } + } + return nil +} + // RemovePath helps with removal path where there is a probability // of running into an executable running that might prevent removal // on Windows. diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index ad5a6b9ad5c..f5932d8023b 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -18,8 +18,6 @@ func isBlockingOnExe(_ error) bool { func scheduleDeleteOnReboot(_ *logp.Logger, _ error, _ string) error { return nil } -func cleanupLeftoverRenames(_ *logp.Logger, _ string) error { return nil } - func isRetryableError(_ error) bool { return false } diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index c9eb6900185..1a2877f208a 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -12,7 +12,6 @@ import ( "io/fs" "os" "path/filepath" - "strings" "syscall" "time" @@ -21,10 +20,6 @@ import ( "github.com/elastic/elastic-agent-libs/logp" ) -// leftoverPrefix is used to rename blocked executables during uninstall. -// Files with this prefix placed at the install root are cleaned up on the next install. -const leftoverPrefix = ".elastic-agent-leftover" - func isBlockingOnExe(err error) bool { if err == nil { return false @@ -88,29 +83,6 @@ func markDeleteOnReboot(path string) error { return windows.MoveFileEx(p, nil, windows.MOVEFILE_DELAY_UNTIL_REBOOT) } -// cleanupLeftoverRenames removes any files at the top level of topPath left -// behind by a previous uninstall's scheduleDeleteOnReboot (files matching -// leftoverPrefix). By the time a new install runs, the old process is gone -// and these files can be deleted normally. -func cleanupLeftoverRenames(log *logp.Logger, topPath string) error { - entries, err := os.ReadDir(topPath) - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return fmt.Errorf("failed to read %q: %w", topPath, err) - } - for _, e := range entries { - if strings.HasPrefix(e.Name(), leftoverPrefix) { - path := filepath.Join(topPath, e.Name()) - if err := os.Remove(path); err != nil { - log.Warnf("Failed to remove leftover file %q from a previous uninstall: %v. You may need to delete it manually.", path, err) - } - } - } - return nil -} - func getPathFromError(blockingErr error) (string, syscall.Errno) { var perr *fs.PathError if errors.As(blockingErr, &perr) {