Improve installed.json handling in v2/tools (#983)

* Introduce mutex policy to the v2 tools and use it to write the installed.json

* Use a map to store installed.json information

* Use the correct lock for reading and writing

* Include the v2.Tools constructor in the old Tools struct

* Add corrupted json test

Author: MatteoPologruto

tools/download.go

@@ -25,7 +25,6 @@ import (
 
 	"github.com/arduino/arduino-create-agent/gen/tools"
 	"github.com/arduino/arduino-create-agent/utilities"
-	"github.com/arduino/arduino-create-agent/v2/pkgs"
 )
 
 // Download will parse the index at the indexURL for the tool to download.
@@ -45,8 +44,8 @@ import (
 // if it already exists.
 func (t *Tools) Download(pack, name, version, behaviour string) error {
 
-	tool := pkgs.New(t.index, t.directory.String(), behaviour)
-	_, err := tool.Install(context.Background(), &tools.ToolPayload{Name: name, Version: version, Packager: pack})
+	t.tools.SetBehaviour(behaviour)
+	_, err := t.tools.Install(context.Background(), &tools.ToolPayload{Name: name, Version: version, Packager: pack})
 	if err != nil {
 		return err
 	}

tools/download_test.go

@@ -160,3 +160,23 @@ func TestDownload(t *testing.T) {
 		})
 	}
 }
+
+func TestCorruptedInstalled(t *testing.T) {
+	// prepare the test environment
+	tempDir := t.TempDir()
+	tempDirPath := paths.New(tempDir)
+	testIndex := index.Resource{
+		IndexFile:   *paths.New("testdata", "test_tool_index.json"),
+		LastRefresh: time.Now(),
+	}
+	corruptedJSON := tempDirPath.Join("installed.json")
+	fileJSON, err := corruptedJSON.Create()
+	require.NoError(t, err)
+	defer fileJSON.Close()
+	_, err = fileJSON.Write([]byte("Hello"))
+	require.NoError(t, err)
+	testTools := New(tempDirPath, &testIndex, func(msg string) { t.Log(msg) })
+	// Download the tool
+	err = testTools.Download("arduino-test", "avrdude", "6.3.0-arduino17", "keep")
+	require.NoError(t, err)
+}

tools/tools.go

@@ -22,6 +22,7 @@ import (
 	"sync"
 
 	"github.com/arduino/arduino-create-agent/index"
+	"github.com/arduino/arduino-create-agent/v2/pkgs"
 	"github.com/arduino/go-paths-helper"
 	"github.com/xrash/smetrics"
 )
@@ -47,6 +48,7 @@ type Tools struct {
 	logger    func(msg string)
 	installed map[string]string
 	mutex     sync.RWMutex
+	tools     *pkgs.Tools
 }
 
 // New will return a Tool object, allowing the caller to execute operations on it.
@@ -60,6 +62,7 @@ func New(directory *paths.Path, index *index.Resource, logger func(msg string))
 		logger:    logger,
 		installed: map[string]string{},
 		mutex:     sync.RWMutex{},
+		tools:     pkgs.New(index, directory.String(), "replace"),
 	}
 	_ = t.readMap()
 	return t

v2/pkgs/tools.go

@@ -29,6 +29,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
 
 	"github.com/arduino/arduino-create-agent/gen/tools"
 	"github.com/arduino/arduino-create-agent/index"
@@ -60,17 +61,23 @@ type Tools struct {
 	index     *index.Resource
 	folder    string
 	behaviour string
+	installed map[string]string
+	mutex     sync.RWMutex
 }
 
 // New will return a Tool object, allowing the caller to execute operations on it.
 // The New function will accept an index as parameter (used to download the indexes)
 // and a folder used to download the indexes
 func New(index *index.Resource, folder, behaviour string) *Tools {
-	return &Tools{
+	t := &Tools{
 		index:     index,
 		folder:    folder,
 		behaviour: behaviour,
+		installed: map[string]string{},
+		mutex:     sync.RWMutex{},
 	}
+	t.readInstalled()
+	return t
 }
 
 // Installedhead is here only because it was required by the front-end.
@@ -181,13 +188,10 @@ func (t *Tools) Install(ctx context.Context, payload *tools.ToolPayload) (*tools
 	key := correctTool.Name + "-" + correctTool.Version
 	// Check if it already exists
 	if t.behaviour == "keep" && pathExists(t.folder) {
-		location, ok, err := checkInstalled(t.folder, key)
-		if err != nil {
-			return nil, err
-		}
+		location, ok := t.installed[key]
 		if ok && pathExists(location) {
 			// overwrite the default tool with this one
-			err := writeInstalled(t.folder, path)
+			err := t.writeInstalled(path)
 			if err != nil {
 				return nil, err
 			}
@@ -245,7 +249,7 @@ func (t *Tools) install(ctx context.Context, path, url, checksum string) (*tools
 	}
 
 	// Write installed.json for retrocompatibility with v1
-	err = writeInstalled(t.folder, path)
+	err = t.writeInstalled(path)
 	if err != nil {
 		return nil, err
 	}
@@ -285,61 +289,53 @@ func rename(base string) extract.Renamer {
 	}
 }
 
-func readInstalled(installedFile string) (map[string]string, error) {
+func (t *Tools) readInstalled() error {
+	t.mutex.RLock()
+	defer t.mutex.RUnlock()
 	// read installed.json
-	installed := map[string]string{}
-	data, err := os.ReadFile(installedFile)
-	if err == nil {
-		err = json.Unmarshal(data, &installed)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return installed, nil
-}
-
-func checkInstalled(folder, key string) (string, bool, error) {
-	installedFile, err := utilities.SafeJoin(folder, "installed.json")
-	if err != nil {
-		return "", false, err
-	}
-	installed, err := readInstalled(installedFile)
-	if err != nil {
-		return "", false, err
-	}
-	location, ok := installed[key]
-	return location, ok, err
-}
-
-func writeInstalled(folder, path string) error {
-	// read installed.json
-	installedFile, err := utilities.SafeJoin(folder, "installed.json")
+	installedFile, err := utilities.SafeJoin(t.folder, "installed.json")
 	if err != nil {
 		return err
 	}
-	installed, err := readInstalled(installedFile)
+	data, err := os.ReadFile(installedFile)
 	if err != nil {
 		return err
 	}
+	return json.Unmarshal(data, &t.installed)
+}
+
+func (t *Tools) writeInstalled(path string) error {
+	t.mutex.Lock()
+	defer t.mutex.Unlock()
 
 	parts := strings.Split(path, string(filepath.Separator))
 	tool := parts[len(parts)-2]
 	toolWithVersion := fmt.Sprint(tool, "-", parts[len(parts)-1])
-	toolFile, err := utilities.SafeJoin(folder, path)
+	toolFile, err := utilities.SafeJoin(t.folder, path)
 	if err != nil {
 		return err
 	}
-	installed[tool] = toolFile
-	installed[toolWithVersion] = toolFile
+	t.installed[tool] = toolFile
+	t.installed[toolWithVersion] = toolFile
 
-	data, err := json.Marshal(installed)
+	data, err := json.Marshal(t.installed)
+	if err != nil {
+		return err
+	}
+
+	installedFile, err := utilities.SafeJoin(t.folder, "installed.json")
 	if err != nil {
 		return err
 	}
 
 	return os.WriteFile(installedFile, data, 0644)
 }
 
+// SetBehaviour sets the download behaviour to either keep or replace
+func (t *Tools) SetBehaviour(behaviour string) {
+	t.behaviour = behaviour
+}
+
 func pathExists(path string) bool {
 	_, err := os.Stat(path)
 	if err == nil {