From 5deff18be660176e09bd0571be6e0614316f6d5c Mon Sep 17 00:00:00 2001 From: Felix Blass Date: Fri, 17 May 2024 11:56:13 +0200 Subject: [PATCH 1/4] Ignore non-existent files when deleting temp-files --- util/configv3/load_config.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/util/configv3/load_config.go b/util/configv3/load_config.go index 73048820ab..49386ead33 100644 --- a/util/configv3/load_config.go +++ b/util/configv3/load_config.go @@ -2,6 +2,7 @@ package configv3 import ( "encoding/json" + "errors" "io/ioutil" "math" "os" @@ -177,7 +178,8 @@ func removeOldTempConfigFiles() error { for _, oldTempFileName := range oldTempFileNames { err = os.Remove(oldTempFileName) - if err != nil { + // ignore if file doesn't exist anymore due to race conditions if multiple cli commands are running + if err != nil && !errors.Is(err, os.ErrNotExist) { return err } } From afa9d17b0be6b0a6ee088e2aeb131263a8e03533 Mon Sep 17 00:00:00 2001 From: Felix Blass Date: Fri, 17 May 2024 14:07:05 +0200 Subject: [PATCH 2/4] Only delete files older than 5min --- util/configv3/load_config.go | 11 ++++++- util/configv3/load_config_test.go | 53 +++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/util/configv3/load_config.go b/util/configv3/load_config.go index 49386ead33..f03d3d773e 100644 --- a/util/configv3/load_config.go +++ b/util/configv3/load_config.go @@ -7,6 +7,7 @@ import ( "math" "os" "path/filepath" + "time" "code.cloudfoundry.org/cli/command/translatableerror" "golang.org/x/crypto/ssh/terminal" @@ -177,8 +178,16 @@ func removeOldTempConfigFiles() error { } for _, oldTempFileName := range oldTempFileNames { + fi, err := os.Lstat(oldTempFileName) + // ignore if file doesn't exist anymore due to race conditions if multiple cli commands are running in parallel + if err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + // only delete old orphans which are not caught by the signal handler in WriteConfig + if fi.ModTime().After(time.Now().Add(-5 * time.Minute)) { + continue + } err = os.Remove(oldTempFileName) - // ignore if file doesn't exist anymore due to race conditions if multiple cli commands are running if err != nil && !errors.Is(err, os.ErrNotExist) { return err } diff --git a/util/configv3/load_config_test.go b/util/configv3/load_config_test.go index 9fdb6a9286..36045d9bf1 100644 --- a/util/configv3/load_config_test.go +++ b/util/configv3/load_config_test.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "os" "path/filepath" + "time" "code.cloudfoundry.org/cli/command/translatableerror" "code.cloudfoundry.org/cli/integration/helpers" @@ -60,22 +61,48 @@ var _ = Describe("Config", func() { }) When("there are old temp-config* files lingering from previous failed attempts to write the config", func() { - BeforeEach(func() { - configDir := filepath.Join(homeDir, ".cf") - Expect(os.MkdirAll(configDir, 0777)).To(Succeed()) - for i := 0; i < 3; i++ { - tmpFile, fileErr := ioutil.TempFile(configDir, "temp-config") - Expect(fileErr).ToNot(HaveOccurred()) - tmpFile.Close() - } + Context("and the files are younger than 5 minutes", func() { + BeforeEach(func() { + configDir := filepath.Join(homeDir, ".cf") + Expect(os.MkdirAll(configDir, 0777)).To(Succeed()) + for i := 0; i < 3; i++ { + configDir := filepath.Join(homeDir, ".cf") + tmpFile, fileErr := ioutil.TempFile(configDir, "temp-config") + Expect(fileErr).ToNot(HaveOccurred()) + tmpFile.Close() + } + }) + + It("keeps the files", func() { + Expect(loadErr).ToNot(HaveOccurred()) + + oldTempFileNames, configErr := filepath.Glob(filepath.Join(homeDir, ".cf", "temp-config?*")) + Expect(configErr).ToNot(HaveOccurred()) + Expect(oldTempFileNames).To(HaveLen(3)) + }) }) - It("removes the lingering temp-config* files", func() { - Expect(loadErr).ToNot(HaveOccurred()) + Context("and the files are older than 5 minutes", func() { + BeforeEach(func() { + configDir := filepath.Join(homeDir, ".cf") + Expect(os.MkdirAll(configDir, 0777)).To(Succeed()) + for i := 0; i < 3; i++ { + tmpFile, fileErr := ioutil.TempFile(configDir, "temp-config") + Expect(fileErr).ToNot(HaveOccurred()) + tmpFile.Close() + oldTime := time.Now().Add(-time.Minute * 10) + err := os.Chtimes(tmpFile.Name(), oldTime, oldTime) + Expect(err).ToNot(HaveOccurred()) + } + }) - oldTempFileNames, configErr := filepath.Glob(filepath.Join(homeDir, ".cf", "temp-config?*")) - Expect(configErr).ToNot(HaveOccurred()) - Expect(oldTempFileNames).To(BeEmpty()) + It("removes the lingering temp-config* files", func() { + Expect(loadErr).ToNot(HaveOccurred()) + + oldTempFileNames, configErr := filepath.Glob(filepath.Join(homeDir, ".cf", "temp-config?*")) + Expect(configErr).ToNot(HaveOccurred()) + Expect(oldTempFileNames).To(BeEmpty()) + }) }) }) From 211429f26b7d9c2269fb589c4f9fc1e48df40750 Mon Sep 17 00:00:00 2001 From: Felix Blass Date: Fri, 17 May 2024 22:58:16 +0200 Subject: [PATCH 3/4] Fix integration test --- integration/shared/isolated/config_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/integration/shared/isolated/config_test.go b/integration/shared/isolated/config_test.go index f8dc92da99..54438b21a8 100644 --- a/integration/shared/isolated/config_test.go +++ b/integration/shared/isolated/config_test.go @@ -2,7 +2,9 @@ package isolated import ( "io/ioutil" + "os" "path/filepath" + "time" helpers "code.cloudfoundry.org/cli/integration/helpers" . "github.com/onsi/ginkgo/v2" @@ -43,6 +45,9 @@ var _ = Describe("Config", func() { tmpFile, err := ioutil.TempFile(configDir, "temp-config") Expect(err).ToNot(HaveOccurred()) tmpFile.Close() + oldTime := time.Now().Add(-time.Minute * 10) + err = os.Chtimes(tmpFile.Name(), oldTime, oldTime) + Expect(err).ToNot(HaveOccurred()) } }) From e2aa3f664787b5c99989ed61cab5f04e80eead25 Mon Sep 17 00:00:00 2001 From: Felix Blass Date: Fri, 17 May 2024 23:24:24 +0200 Subject: [PATCH 4/4] Fix panic if file was deleted before os.Lstat --- util/configv3/load_config.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/util/configv3/load_config.go b/util/configv3/load_config.go index f03d3d773e..7642da776a 100644 --- a/util/configv3/load_config.go +++ b/util/configv3/load_config.go @@ -179,8 +179,11 @@ func removeOldTempConfigFiles() error { for _, oldTempFileName := range oldTempFileNames { fi, err := os.Lstat(oldTempFileName) - // ignore if file doesn't exist anymore due to race conditions if multiple cli commands are running in parallel - if err != nil && !errors.Is(err, os.ErrNotExist) { + if err != nil { + // ignore if file doesn't exist anymore due to race conditions if multiple cli commands are running in parallel + if errors.Is(err, os.ErrNotExist) { + continue + } return err } // only delete old orphans which are not caught by the signal handler in WriteConfig