Add file locking to dir cache

src/build/build_step.go

@@ -210,7 +210,10 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 	} else {
 		// Wait if another process is currently building this target
 		state.LogBuildResult(target, core.TargetBuilding, "Acquiring target lock...")
-		file := core.AcquireExclusiveFileLock(target.BuildLockFile())
+		file, err := core.AcquireExclusiveFileLock(target.BuildLockFile())
+		if err != nil {
+			return err
+		}
 		defer core.ReleaseFileLock(file)
 		state.LogBuildResult(target, core.TargetBuilding, "Preparing...")
 

src/cache/dir_cache.go

@@ -7,6 +7,7 @@ import (
 	"bufio"
 	"compress/gzip"
 	"encoding/base64"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -34,6 +35,13 @@ type dirCache struct {
 
 func (cache *dirCache) Store(target *core.BuildTarget, key []byte, files []string) {
 	cacheDir := cache.getPath(target, key, "")
+	lockFile, err := core.AcquireExclusiveFileLock(cacheDir + ".lock")
+	if err != nil {
+		log.Warning("Failed to acquire cache lock for %s, will not store", target)
+		return
+	}
+	defer core.ReleaseFileLock(lockFile)
+
 	tmpDir := cache.getFullPath(target, key, "", "=")
 	cache.markDir(cacheDir, 0)
 	if err := fs.RemoveAll(cacheDir); err != nil {
@@ -178,6 +186,13 @@ func (cache *dirCache) storeFile(target *core.BuildTarget, out, cacheDir string)
 }
 
 func (cache *dirCache) Retrieve(target *core.BuildTarget, key []byte, outs []string) bool {
+	lockFile, err := core.AcquireSharedFileLock(cache.getPath(target, key, "") + ".lock")
+	if err != nil {
+		log.Warning("Failed to acquire cache lock for %s, will not retrieve", target)
+		return false
+	}
+	defer core.ReleaseFileLock(lockFile)
+
 	return cache.retrieve(target, key, "", outs)
 }
 
@@ -443,14 +458,8 @@ func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 {
 		}
 
 		log.Debug("Cleaning %s, accessed %s, saves %s", entry.Path, humanize.Time(time.Unix(entry.Atime, 0)), humanize.Bytes(entry.Size))
-		// Try to rename the directory first so we don't delete bits while someone might access them.
-		newPath := entry.Path + "="
-		if err := os.Rename(entry.Path, newPath); err != nil {
-			log.Errorf("Couldn't rename %s: %s", entry.Path, err)
-			continue
-		}
-		if err := fs.RemoveAll(newPath); err != nil {
-			log.Errorf("Couldn't remove %s: %s", newPath, err)
+		if err := cache.cleanPath(entry.Path); err != nil {
+			log.Warning("Error while cleaning cache: %s", err)
 			continue
 		}
 		totalSize -= entry.Size
@@ -461,6 +470,24 @@ func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 {
 	return totalSize
 }
 
+func (cache *dirCache) cleanPath(path string) error {
+	lockFile, err := core.AcquireExclusiveFileLock(path + ".lock")
+	if err != nil {
+		return err
+	}
+	defer core.ReleaseFileLock(lockFile)
+
+	// Try to rename the directory first so if anything goes wrong we leave it inaccessible to anyone else
+	newPath := path + "="
+	if err := os.Rename(path, newPath); err != nil {
+		return fmt.Errorf("Couldn't rename %s: %w", path, err)
+	}
+	if err := fs.RemoveAll(newPath); err != nil {
+		return fmt.Errorf("Couldn't remove %s: %w", newPath, err)
+	}
+	return nil
+}
+
 // shouldClean returns true if we should clean this file.
 // We track this in order to clean only entire entries in the cache, not just individual files from them.
 func (cache *dirCache) shouldClean(name string, isDir bool) bool {

src/core/lock.go

@@ -74,13 +74,14 @@ func openRepoLockFile() error {
 }
 
 // AcquireExclusiveFileLock opens a file to acquire an exclusive lock.
-// Dies if the lock cannot be successfully acquired.
-func AcquireExclusiveFileLock(filePath string) *os.File {
-	lockFile, err := acquireOpenFileLock(filePath, syscall.LOCK_EX)
-	if err != nil {
-		log.Fatal(err)
-	}
-	return lockFile
+func AcquireExclusiveFileLock(filePath string) (*os.File, error) {
+	return acquireOpenFileLock(filePath, syscall.LOCK_EX)
+}
+
+// AcquireSharedFileLock opens a file to acquire a shared lock.
+// Multiple of these can be held at once, but not concurrently with an exclusive lock (ala a RWMutex or similar).
+func AcquireSharedFileLock(filePath string) (*os.File, error) {
+	return acquireOpenFileLock(filePath, syscall.LOCK_SH)
 }
 
 // Base function that allows to set up different lock modes and facilitate testing.
@@ -132,9 +133,11 @@ func acquireFileLock(file *os.File, how int, levelLog logFunc) error {
 	}
 	log.Debug("Acquired lock for %s", file.Name())
 
-	// Record content
-	if err := file.Truncate(0); err == nil {
-		file.WriteAt([]byte(strconv.Itoa(os.Getpid())), 0)
+	// Record content, only if we have an exclusive lock.
+	if how&syscall.LOCK_EX != 0 {
+		if err := file.Truncate(0); err == nil {
+			file.WriteAt([]byte(strconv.Itoa(os.Getpid())), 0)
+		}
 	}
 
 	return nil

src/core/lock_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestOpenLockFile(t *testing.T) {
@@ -42,12 +43,12 @@ func TestAcquireSharedRepoRoot(t *testing.T) {
 	assert.IsType(t, &os.File{}, repoLockFile)
 
 	contents, err := os.ReadFile(repoLockFile.Name())
-	assert.Equal(t, strconv.Itoa(os.Getpid()), string(contents))
-	assert.NoError(t, err)
+	require.NoError(t, err)
+	assert.Empty(t, contents)
 }
 
 func TestAcquireExclusiveRepoRoot(t *testing.T) {
-	AcquireSharedRepoLock()
+	AcquireExclusiveRepoLock()
 	defer ReleaseRepoLock()
 
 	assert.IsType(t, &os.File{}, repoLockFile)
@@ -133,7 +134,8 @@ func TestReleaseRepoLock(t *testing.T) {
 }
 
 func TestAcquireExclusiveFileLock(t *testing.T) {
-	file := AcquireExclusiveFileLock("path/to/file")
+	file, err := AcquireExclusiveFileLock("path/to/file")
+	require.NoError(t, err)
 	defer ReleaseFileLock(file)
 
 	assert.IsType(t, &os.File{}, file)
@@ -162,9 +164,10 @@ func TestAcquireExclusiveFileLockTwice(t *testing.T) {
 }
 
 func TestReleaseFileLock(t *testing.T) {
-	file := AcquireExclusiveFileLock("path/to/file")
+	file, err := AcquireExclusiveFileLock("path/to/file")
+	require.NoError(t, err)
 
 	ReleaseFileLock(file)
-	err := file.Close()
+	err = file.Close()
 	assert.Error(t, err, "file already closed")
 }

src/fs/fs.go

@@ -64,20 +64,11 @@ func IsSymlink(filename string) bool {
 // IsSameFile returns true if two filenames describe the same underlying file
 // (i.e. inode for Unix and potentially file path names for other OS's)
 func IsSameFile(a, b string) bool {
-	i1, err1 := getFileInfo(a)
-	i2, err2 := getFileInfo(b)
+	i1, err1 := os.Stat(a)
+	i2, err2 := os.Stat(b)
 	return err1 == nil && err2 == nil && os.SameFile(i1, i2)
 }
 
-// getFileInfo returns the FileInfo of a file.
-func getFileInfo(filename string) (os.FileInfo, error) {
-	fi, err := os.Stat(filename)
-	if err != nil {
-		return nil, err
-	}
-	return fi, nil
-}
-
 // CopyFile copies a file from 'from' to 'to', with an attempt to perform a copy & rename
 // to avoid chaos if anything goes wrong partway.
 func CopyFile(from string, to string, mode os.FileMode) error {

src/remote/remote.go

@@ -455,7 +455,10 @@ func (c *Client) Download(target *core.BuildTarget) error {
 	}
 	return c.download(target, func() error {
 		buildAction := c.unstampedBuildActionDigests.Get(target.Label)
-		file := core.AcquireExclusiveFileLock(target.BuildLockFile())
+		file, err := core.AcquireExclusiveFileLock(target.BuildLockFile())
+		if err != nil {
+			return err
+		}
 		defer core.ReleaseFileLock(file)
 
 		// This is a bit of a grungy hack to avoid clobbering outputs.
@@ -465,7 +468,10 @@ func (c *Client) Download(target *core.BuildTarget) error {
 				if l, ok := t.Label(); ok {
 					if l.PackageName == target.Label.PackageName && l.Subrepo == target.Label.Subrepo {
 						t := c.state.Graph.TargetOrDie(l)
-						file := core.AcquireExclusiveFileLock(t.BuildLockFile())
+						file, err := core.AcquireExclusiveFileLock(t.BuildLockFile())
+						if err != nil {
+							return err
+						}
 						defer core.ReleaseFileLock(file)
 					}
 				}

src/test/test_step.go

@@ -169,7 +169,11 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge
 
 	// Wait if another process is currently testing this target
 	state.LogTestRunning(target, run, core.TargetTesting, "Acquiring target lock...")
-	file := core.AcquireExclusiveFileLock(target.TestLockFile(run))
+	file, err := core.AcquireExclusiveFileLock(target.TestLockFile(run))
+	if err != nil {
+		state.LogBuildError(label, core.TargetTestFailed, err, "Failed to acquire target lock")
+		return
+	}
 	defer core.ReleaseFileLock(file)
 	state.LogTestRunning(target, run, core.TargetTesting, "Testing...")