From 3eec49b3719e308e4e4c35381b9b12cfda880875 Mon Sep 17 00:00:00 2001
From: f-blass <42929142+f-blass@users.noreply.github.com>
Date: Wed, 22 May 2024 16:30:25 +0200
Subject: [PATCH] Fix race condition in config handling (main) (#2937)

* Ignore non-existent files when deleting temp-files

* Only delete files older than 5min
---
 integration/shared/isolated/config_test.go |  5 ++
 util/configv3/load_config.go               | 16 ++++++-
 util/configv3/load_config_test.go          | 53 ++++++++++++++++------
 3 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/integration/shared/isolated/config_test.go b/integration/shared/isolated/config_test.go
index f8dc92da996..54438b21a8f 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())
 				}
 			})
 
diff --git a/util/configv3/load_config.go b/util/configv3/load_config.go
index 90470e2464d..e40c686dd90 100644
--- 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
index 9fdb6a9286b..36045d9bf1c 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())
+				})
 			})
 		})