Merge pull request #593 from numtide/fix/concurrent-invocation

fix: force-remove db file when clearing cache

Author: brianmcgee

cmd/format/format.go

@@ -77,6 +77,13 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 		}()
 	}
 
+	// Remove the cache first before potentially opening a new one.
+	if cfg.ClearCache {
+		if err := cache.Remove(cfg.TreeRoot); err != nil {
+			return fmt.Errorf("failed to clear cache: %w", err)
+		}
+	}
+
 	var db *bolt.DB
 
 	// open the db unless --no-cache was specified
@@ -94,15 +101,6 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 		}()
 	}
 
-	if db != nil {
-		// clear the cache if desired
-		if cfg.ClearCache {
-			if err = cache.Clear(db); err != nil {
-				return fmt.Errorf("failed to clear cache: %w", err)
-			}
-		}
-	}
-
 	// create an overall app context
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()

cmd/root_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/numtide/treefmt/v2/walk"
 	cp "github.com/otiai10/copy"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sync/errgroup"
 )
 
 func TestOnUnmatched(t *testing.T) {
@@ -2141,6 +2142,79 @@ func TestProjectRootIsSymlink(t *testing.T) {
 	)
 }
 
+func TestConcurrentInvocation(t *testing.T) {
+	as := require.New(t)
+
+	tempDir := test.TempExamples(t)
+	configPath := filepath.Join(tempDir, "/treefmt.toml")
+
+	test.ChangeWorkDir(t, tempDir)
+
+	cfg := &config.Config{
+		FormatterConfigs: map[string]*config.Formatter{
+			"echo": {
+				Command:  "echo",
+				Includes: []string{"*"},
+			},
+			"slow": {
+				Command: "test-fmt-delayed-append",
+				// connect timeout for the db is 1 second
+				// wait 2 seconds before appending ' ' to each provided path
+				Options:  []string{"2", " "},
+				Includes: []string{"*"},
+			},
+		},
+	}
+
+	eg := errgroup.Group{}
+
+	// concurrent invocation with one slow instance and one not
+
+	eg.Go(func() error {
+		treefmt(t,
+			withArgs("--formatters", "slow"),
+			withConfig(configPath, cfg),
+			withNoError(t),
+		)
+
+		return nil
+	})
+
+	time.Sleep(500 * time.Millisecond)
+
+	treefmt(t,
+		withArgs("--formatters", "echo"),
+		withConfig(configPath, cfg),
+		withError(func(as *require.Assertions, err error) {
+			as.ErrorContains(err, "failed to open cache")
+		}),
+	)
+
+	as.NoError(eg.Wait())
+
+	// concurrent invocation with one slow instance and one configured to clear the cache
+
+	eg.Go(func() error {
+		treefmt(t,
+			withArgs("--formatters", "slow"),
+			withConfig(configPath, cfg),
+			withNoError(t),
+		)
+
+		return nil
+	})
+
+	time.Sleep(500 * time.Millisecond)
+
+	treefmt(t,
+		withArgs("-c", "--formatters", "echo"),
+		withConfig(configPath, cfg),
+		withNoError(t),
+	)
+
+	as.NoError(eg.Wait())
+}
+
 type options struct {
 	args []string
 	env  map[string]string

nix/packages/treefmt/formatters.nix

@@ -45,4 +45,16 @@ with pkgs; [
       done
     '';
   })
+  (pkgs.writeShellApplication {
+    name = "test-fmt-delayed-append";
+    text = ''
+      DELAY="$1"
+      shift
+
+      # sleep first
+      sleep "$DELAY"
+
+      test-fmt-append "$@"
+    '';
+  })
 ]

walk/cache/cache.go

@@ -4,6 +4,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"os"
 	"time"
 
 	"github.com/adrg/xdg"
@@ -14,26 +15,33 @@ const (
 	bucketPaths = "paths"
 )
 
-func Open(root string) (*bolt.DB, error) {
-	var (
-		err  error
-		path string
-	)
-
-	// Otherwise, the database will be located in `XDG_CACHE_DIR/treefmt/eval-cache/<name>.db`, where <name> is
-	// determined by hashing the treeRoot path.
-	// This associates a given treeRoot with a given instance of the cache.
+// Path returns a unique local cache file path for the given root string, using its SHA-256 hash.
+func Path(root string) (string, error) {
 	digest := sha256.Sum256([]byte(root))
 
 	name := hex.EncodeToString(digest[:])
-	if path, err = xdg.CacheFile(fmt.Sprintf("treefmt/eval-cache/%v.db", name)); err != nil {
-		return nil, fmt.Errorf("could not resolve local path for the cache: %w", err)
+
+	path, err := xdg.CacheFile(fmt.Sprintf("treefmt/eval-cache/%v.db", name))
+	if err != nil {
+		return "", fmt.Errorf("could not resolve local path for the cache: %w", err)
+	}
+
+	return path, nil
+}
+
+// Open initialises and opens a Bolt database for the specified root path.
+// Returns a pointer to the opened database or an error if initialisation fails.
+func Open(root string) (*bolt.DB, error) {
+	// determine the db location
+	path, err := Path(root)
+	if err != nil {
+		return nil, err
 	}
 
 	// open db
 	db, err := bolt.Open(path, 0o600, &bolt.Options{Timeout: 1 * time.Second})
 	if err != nil {
-		return nil, fmt.Errorf("failed to open cache db: %w", err)
+		return nil, fmt.Errorf("failed to open cache db at %s: %w", path, err)
 	}
 
 	// ensure bucket exist
@@ -56,28 +64,19 @@ func PathsBucket(tx *bolt.Tx) *bolt.Bucket {
 	return tx.Bucket([]byte("paths"))
 }
 
-func deleteAll(bucket *bolt.Bucket) error {
-	c := bucket.Cursor()
-	for k, v := c.First(); !(k == nil && v == nil); k, v = c.Next() {
-		if err := c.Delete(); err != nil {
-			return fmt.Errorf("failed to remove cache entry for key %s: %w", string(k), err)
-		}
+func Remove(root string) error {
+	// determine the db location
+	path, err := Path(root)
+	if err != nil {
+		return err
 	}
 
-	return nil
-}
-
-func Clear(db *bolt.DB) error {
-	err := db.Update(func(tx *bolt.Tx) error {
-		err := deleteAll(PathsBucket(tx))
-		if err != nil {
-			return fmt.Errorf("failed to clear cache: %w", err)
-		}
-
-		return nil
-	})
-	if err != nil {
-		return fmt.Errorf("failed to clear cache: %w", err)
+	// Remove any db which might already exist.
+	// If a treefmt process is currently running with a db open at the same location, it will continue to function
+	// as normal, however, when it exits the disk space its inode was referencing will be reclaimed.
+	// This will not work on Windows if we ever support it.
+	if err = os.Remove(path); !(err == nil || os.IsNotExist(err)) {
+		return fmt.Errorf("failed to remove cache db at %s: %w", path, err)
 	}
 
 	return nil