diff --git a/changelog/fragments/1695050880-Improve-retry-strategy-when-uninstalling-agent.yaml b/changelog/fragments/1695050880-Improve-retry-strategy-when-uninstalling-agent.yaml new file mode 100644 index 00000000000..b3c1e7ac5e7 --- /dev/null +++ b/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 diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index 612a44587de..cc1f18380f3 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/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 { diff --git a/internal/pkg/agent/install/uninstall_windows_test.go b/internal/pkg/agent/install/uninstall_windows_test.go index 513a8a3f622..a3a9f3a914f 100644 --- a/internal/pkg/agent/install/uninstall_windows_test.go +++ b/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) }