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 fe624d3d34..86e3af0c8d 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" @@ -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 { diff --git a/src/core/lock.go b/src/core/lock.go index 19268e9d2b..629cb12e5b 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_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 diff --git a/src/core/lock_test.go b/src/core/lock_test.go index ca53bfa669..cd403896dd 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) { @@ -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() - assert.Error(t, err, "file already closed") + err = file.Close() + assert.ErrorIs(t, err, os.ErrClosed) } 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...")