Fix race condition in config handling (main) (#2937)

* Ignore non-existent files when deleting temp-files * Only delete files older than 5min
cloudfoundry · May 22, 2024 · 3eec49b · 3eec49b
1 parent e9b1f00
commit 3eec49b
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 14 deletions.
diff --git 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())
 				}
 			})
 

diff --git a/util/configv3/load_config.go b/util/configv3/load_config.go
@@ -2,10 +2,12 @@ package configv3
 
 import (
 	"encoding/json"
+	"errors"
 	"io/ioutil"
 	"math"
 	"os"
 	"path/filepath"
+	"time"
 
 	"code.cloudfoundry.org/cli/command/translatableerror"
 	"golang.org/x/crypto/ssh/terminal"
@@ -182,8 +184,20 @@ func removeOldTempConfigFiles() error {
 	}
 
 	for _, oldTempFileName := range oldTempFileNames {
-		err = os.Remove(oldTempFileName)
+		fi, err := os.Lstat(oldTempFileName)
 		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
+		if fi.ModTime().After(time.Now().Add(-5 * time.Minute)) {
+			continue
+		}
+		err = os.Remove(oldTempFileName)
+		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
@@ -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())
+				})
 			})
 		})