From 06050499b933afe0a897334c2b80df6c47226c79 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 14 Aug 2026 13:39:20 +0100 Subject: [PATCH 1/6] Add file locking to dir cache --- src/cache/dir_cache.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/cache/dir_cache.go b/src/cache/dir_cache.go index fe624d3d34..7904f2341c 100644 --- a/src/cache/dir_cache.go +++ b/src/cache/dir_cache.go @@ -33,6 +33,9 @@ type dirCache struct { } func (cache *dirCache) Store(target *core.BuildTarget, key []byte, files []string) { + lockFile := core.AcquireExclusiveFileLock(cache.getFullPath(target, key, "", ".lock")) + defer core.ReleaseFileLock(lockFile) + cacheDir := cache.getPath(target, key, "") tmpDir := cache.getFullPath(target, key, "", "=") cache.markDir(cacheDir, 0) @@ -178,6 +181,9 @@ func (cache *dirCache) storeFile(target *core.BuildTarget, out, cacheDir string) } func (cache *dirCache) Retrieve(target *core.BuildTarget, key []byte, outs []string) bool { + lockFile := core.AcquireExclusiveFileLock(cache.getFullPath(target, key, "", ".lock")) + defer core.ReleaseFileLock(lockFile) + return cache.retrieve(target, key, "", outs) } @@ -443,7 +449,10 @@ 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. + lockFile := core.AcquireExclusiveFileLock(entry.Path + ".lock") + defer core.ReleaseFileLock(lockFile) + + // Try to rename the directory first so if anything goes wrong we leave it inaccessible to anyone else newPath := entry.Path + "=" if err := os.Rename(entry.Path, newPath); err != nil { log.Errorf("Couldn't rename %s: %s", entry.Path, err) From 5ad2a27b0bb0f1534abdf8420316fb343ba4c081 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 14 Aug 2026 14:24:33 +0100 Subject: [PATCH 2/6] Some improvements --- src/build/build_step.go | 5 ++++- src/cache/dir_cache.go | 44 +++++++++++++++++++++++++++++------------ src/core/lock.go | 15 +++++++------- src/fs/fs.go | 13 ++---------- src/remote/remote.go | 10 ++++++++-- src/test/test_step.go | 6 +++++- 6 files changed, 58 insertions(+), 35 deletions(-) diff --git a/src/build/build_step.go b/src/build/build_step.go index 303c75097a..66039449bc 100644 --- a/src/build/build_step.go +++ b/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...") diff --git a/src/cache/dir_cache.go b/src/cache/dir_cache.go index 7904f2341c..c9a5218b84 100644 --- a/src/cache/dir_cache.go +++ b/src/cache/dir_cache.go @@ -7,6 +7,7 @@ import ( "bufio" "compress/gzip" "encoding/base64" + "fmt" "io" "os" "path/filepath" @@ -33,7 +34,11 @@ type dirCache struct { } func (cache *dirCache) Store(target *core.BuildTarget, key []byte, files []string) { - lockFile := core.AcquireExclusiveFileLock(cache.getFullPath(target, key, "", ".lock")) + lockFile, err := core.AcquireExclusiveFileLock(cache.getFullPath(target, key, "", ".lock")) + if err != nil { + log.Warning("Failed to acquire cache lock for %s, will not store", target) + return + } defer core.ReleaseFileLock(lockFile) cacheDir := cache.getPath(target, key, "") @@ -181,7 +186,11 @@ func (cache *dirCache) storeFile(target *core.BuildTarget, out, cacheDir string) } func (cache *dirCache) Retrieve(target *core.BuildTarget, key []byte, outs []string) bool { - lockFile := core.AcquireExclusiveFileLock(cache.getFullPath(target, key, "", ".lock")) + lockFile, err := core.AcquireSharedFileLock(cache.getFullPath(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) @@ -449,17 +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)) - lockFile := core.AcquireExclusiveFileLock(entry.Path + ".lock") - defer core.ReleaseFileLock(lockFile) - - // Try to rename the directory first so if anything goes wrong we leave it inaccessible to anyone else - 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 @@ -470,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 { diff --git a/src/core/lock.go b/src/core/lock.go index 19268e9d2b..734b2891b1 100644 --- a/src/core/lock.go +++ b/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_EX) } // Base function that allows to set up different lock modes and facilitate testing. diff --git a/src/fs/fs.go b/src/fs/fs.go index 03de2d8314..73bb3dfd90 100644 --- a/src/fs/fs.go +++ b/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 { diff --git a/src/remote/remote.go b/src/remote/remote.go index 6840e4adca..0ca4394c47 100644 --- a/src/remote/remote.go +++ b/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) } } diff --git a/src/test/test_step.go b/src/test/test_step.go index 2aa7332f26..f38e44d87a 100644 --- a/src/test/test_step.go +++ b/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...") From 5ab1fb033dc1f005f6a13fdef4754d1700e675db Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 14 Aug 2026 15:56:04 +0100 Subject: [PATCH 3/6] Fix paths --- src/cache/dir_cache.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cache/dir_cache.go b/src/cache/dir_cache.go index c9a5218b84..86e3af0c8d 100644 --- a/src/cache/dir_cache.go +++ b/src/cache/dir_cache.go @@ -34,14 +34,14 @@ type dirCache struct { } func (cache *dirCache) Store(target *core.BuildTarget, key []byte, files []string) { - lockFile, err := core.AcquireExclusiveFileLock(cache.getFullPath(target, key, "", ".lock")) + 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) - cacheDir := cache.getPath(target, key, "") tmpDir := cache.getFullPath(target, key, "", "=") cache.markDir(cacheDir, 0) if err := fs.RemoveAll(cacheDir); err != nil { @@ -186,7 +186,7 @@ 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.getFullPath(target, key, "", ".lock")) + 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 From 3b90b68fa4225a372c0908479f926e6ff3fde087 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 14 Aug 2026 16:01:35 +0100 Subject: [PATCH 4/6] Fix up tests --- src/core/lock_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/core/lock_test.go b/src/core/lock_test.go index ca53bfa669..323b84f65d 100644 --- a/src/core/lock_test.go +++ b/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) { @@ -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") } From 4d0d0d1e569a26d0732f770da8561d7a9fe88d49 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 15 Aug 2026 10:36:11 +0100 Subject: [PATCH 5/6] Don't write file on shared lock --- src/core/lock.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/lock.go b/src/core/lock.go index 734b2891b1..629cb12e5b 100644 --- a/src/core/lock.go +++ b/src/core/lock.go @@ -81,7 +81,7 @@ func AcquireExclusiveFileLock(filePath string) (*os.File, error) { // 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_EX) + return acquireOpenFileLock(filePath, syscall.LOCK_SH) } // Base function that allows to set up different lock modes and facilitate testing. @@ -133,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 From 493b6435cf7533c371ae9b1a8720e6d0fe258da2 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 15 Aug 2026 13:07:47 +0100 Subject: [PATCH 6/6] Fix tests --- src/core/lock_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/lock_test.go b/src/core/lock_test.go index 323b84f65d..e5f921cc67 100644 --- a/src/core/lock_test.go +++ b/src/core/lock_test.go @@ -43,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)