Fix TestRemovePath flakiness (#3431)

Slightly modified the RemovePath retry loop on windows adding another iteration after deleting a blocking executable and introducing a small sleep between attempts.

changelog/fragments/1695050880-Improve-retry-strategy-when-uninstalling-agent.yaml

@@ -0,0 +1,32 @@
+# 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: feature
+
+# Change summary; a 80ish characters long description of the change.
+summary: Improve uninstall by adding some pause between retries when removal is blocked by busy files
+
+# 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:
+
+# Affected component; a word indicating the component this changeset affects.
+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

internal/pkg/agent/install/uninstall.go

@@ -99,27 +99,23 @@ func Uninstall(cfgFile, topPath, uninstallToken string) error {
 func RemovePath(path string) error {
 	const arbitraryTimeout = 5 * time.Second
 	start := time.Now()
-	nextSleep := 1 * time.Millisecond
-	for {
-		err := os.RemoveAll(path)
-		if err == nil {
-			return nil
-		}
-		if isBlockingOnExe(err) {
-			// try to remove the blocking exe
-			err = removeBlockingExe(err)
-		}
-		if err == nil {
-			return nil
-		}
-		if !isRetryableError(err) {
-			return err
+	var lastErr error
+	for time.Since(start) <= arbitraryTimeout {
+		lastErr = os.RemoveAll(path)
+
+		if lastErr == nil || !isRetryableError(lastErr) {
+			return lastErr
 		}
 
-		if d := time.Since(start) + nextSleep; d >= arbitraryTimeout {
-			return err
+		if isBlockingOnExe(lastErr) {
+			// try to remove the blocking exe and try again to clean up the path
+			_ = removeBlockingExe(lastErr)
 		}
+
+		time.Sleep(50 * time.Millisecond)
 	}
+
+	return fmt.Errorf("timed out while removing %q. Last error: %w", path, lastErr)
 }
 
 func RemoveBut(path string, bestEffort bool, exceptions ...string) error {

internal/pkg/agent/install/uninstall_windows_test.go

@@ -7,12 +7,14 @@
 package install
 
 import (
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"testing"
 
 	"github.com/otiai10/copy"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -22,8 +24,6 @@ func TestRemovePath(t *testing.T) {
 		binaryName = pkgName + ".exe"
 	)
 
-	t.Skip("https://github.com/elastic/elastic-agent/issues/3221")
-
 	// 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.
@@ -51,5 +51,7 @@ func TestRemovePath(t *testing.T) {
 
 	// Ensure the directory containing the executable can be removed.
 	err = RemovePath(destDir)
-	require.NoError(t, err)
+	assert.NoError(t, err)
+	_, err = os.Stat(destDir)
+	assert.ErrorIsf(t, err, fs.ErrNotExist, "path %q still exists after removal", destDir)
 }