From 0ef39f2ff4c448ba1d50d315eaeb90aa64e3a8f7 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 9 Oct 2026 16:13:05 +0100 Subject: [PATCH 1/2] Make eviction strategy in dir cache link-aware --- ChangeLog | 3 + go.mod | 1 - go.sum | 2 - src/cache/BUILD | 1 - src/cache/dir_cache.go | 175 +++++++++++++++++++++-------- src/cache/dir_cache_eviction.md | 189 ++++++++++++++++++++++++++++++++ src/cache/dir_cache_test.go | 103 +++++++++++++++-- third_party/go/BUILD | 5 - tools/misc/BUILD | 11 ++ 9 files changed, 422 insertions(+), 68 deletions(-) create mode 100644 src/cache/dir_cache_eviction.md diff --git a/ChangeLog b/ChangeLog index 44e9b7c17f..557918c2e0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -4,6 +4,9 @@ Version 17.34.0 * Check status field of fetch blob response (#3607) * Subinclude cycle detection (#3615) * Fix for `plz export --notrim` in some cases (#3621) + * Dir cache cleaning no longer counts or removes entries that are hard linked into a plz-out, + since removing them frees no space. The watermarks now apply only to unshared files, and + entries are aged by when they were last used rather than their access time. Version 17.33.0 --------------- diff --git a/go.mod b/go.mod index a2ecdc007c..68c96e1b7e 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,6 @@ require ( github.com/cespare/xxhash/v2 v2.3.0 github.com/coreos/go-semver v0.3.1 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc - github.com/djherbis/atime v1.1.0 github.com/dustin/go-humanize v1.0.1 github.com/fsnotify/fsnotify v1.10.1 github.com/golang/protobuf v1.5.4 diff --git a/go.sum b/go.sum index 53eb839683..b54228b159 100644 --- a/go.sum +++ b/go.sum @@ -51,8 +51,6 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/djherbis/atime v1.1.0 h1:rgwVbP/5by8BvvjBNrbh64Qz33idKT3pSnMSJsxhi0g= -github.com/djherbis/atime v1.1.0/go.mod h1:28OF6Y8s3NQWwacXc5eZTsEsiMzp7LF8MbXE+XJPdBE= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= diff --git a/src/cache/BUILD b/src/cache/BUILD index 72d59791b0..dd11cd56aa 100644 --- a/src/cache/BUILD +++ b/src/cache/BUILD @@ -7,7 +7,6 @@ go_library( pgo_file = "//:pgo", visibility = ["PUBLIC"], deps = [ - "///third_party/go/github.com_djherbis_atime//:atime", "///third_party/go/github.com_dustin_go-humanize//:go-humanize", "///third_party/go/github.com_hashicorp_go-retryablehttp//:go-retryablehttp", "//src/clean", diff --git a/src/cache/dir_cache.go b/src/cache/dir_cache.go index 86e3af0c8d..d8e54efad2 100644 --- a/src/cache/dir_cache.go +++ b/src/cache/dir_cache.go @@ -14,9 +14,9 @@ import ( "sort" "strings" "sync" + "syscall" "time" - "github.com/djherbis/atime" "github.com/dustin/go-humanize" "github.com/thought-machine/please/src/clean" @@ -181,8 +181,7 @@ func (cache *dirCache) storeFile(target *core.BuildTarget, out, cacheDir string) } // TODO(peterebden): This is a little inefficient, it would be better to track the size in // RecursiveCopy rather than walking again. - size, _ := findSize(cachedFile) - return size + return unsharedSize(cachedFile) } func (cache *dirCache) Retrieve(target *core.BuildTarget, key []byte, outs []string) bool { @@ -209,11 +208,15 @@ func (cache *dirCache) retrieve(target *core.BuildTarget, key []byte, suffix str } func (cache *dirCache) retrieveFiles(target *core.BuildTarget, cacheDir string, outs []string) (bool, error) { - if !core.PathExists(cacheDir) { + info, err := os.Stat(cacheDir) + if os.IsNotExist(err) { log.Debug("%s: %s doesn't exist in dir cache", target.Label, cacheDir) return false, nil + } else if err != nil { + return false, err } cache.markDir(cacheDir, 0) + cache.touch(cacheDir, info) if len(outs) == 0 { return true, nil } @@ -378,91 +381,158 @@ func newDirCache(config *core.Configuration) *dirCache { return cache } -// Period of time in seconds between which two artifacts are considered to have the same atime. +// Period of time in seconds between which two artifacts are considered to have been used at the same time. const accessTimeGracePeriod = 600 // Ten minutes -// A cacheEntry represents a single file entry in the cache. +// touchInterval is the minimum time between updates of an entry's modification time when it's retrieved. +// Updating it on every retrieval would cause a lot of unnecessary writes. +const touchInterval = time.Hour + +// A cacheEntry represents a single entry in the cache (i.e. one stored target). type cacheEntry struct { Path string - Size uint64 - Atime int64 + Size uint64 // Total size of the files in this entry that are not linked from outside the cache + Mtime int64 + Files []fileID } -func findSize(path string) (uint64, error) { - var totalSize uint64 +// A fileID identifies a file on disk independently of the path(s) it's linked at. +type fileID struct { + Dev, Ino uint64 +} + +// A cacheFile is a single file in the cache, which may be linked into multiple entries +// and also from outside the cache (typically into plz-out, when it's stored or retrieved). +type cacheFile struct { + Size uint64 + Links uint64 // Total number of hard links to this file + Refs uint64 // Number of those links that are inside the cache + Unshared bool // True if this file is only linked from inside the cache. +} + +// fileInfo returns the identifier and number of hard links for a file. +func fileInfo(info os.FileInfo) (fileID, uint64) { + st := info.Sys().(*syscall.Stat_t) + // The types of these fields vary between platforms, so the conversions are needed on some of them. + return fileID{Dev: uint64(st.Dev), Ino: uint64(st.Ino)}, uint64(st.Nlink) //nolint:unconvert +} + +// unsharedSize returns the total size of the files under the given path that are not hard linked from anywhere else. +func unsharedSize(path string) uint64 { + var size uint64 + filepath.Walk(path, func(path string, info os.FileInfo, err error) error { + if err == nil && !info.IsDir() { + if _, links := fileInfo(info); links <= 1 { + size += uint64(info.Size()) + } + } + return nil + }) + return size +} + +// readEntry reads a single entry in the cache, recording all the files in it. +func readEntry(path string, files map[fileID]*cacheFile) (*cacheEntry, error) { + info, err := os.Lstat(path) + if err != nil { + return nil, err + } + entry := &cacheEntry{Path: path, Mtime: info.ModTime().Unix()} if err := filepath.Walk(path, func(path string, info os.FileInfo, err error) error { if err != nil { return err + } else if info.IsDir() { + return nil + } + id, links := fileInfo(info) + if _, present := files[id]; !present { + files[id] = &cacheFile{Size: uint64(info.Size()), Links: links} } - totalSize += uint64(info.Size()) + files[id].Refs++ + entry.Files = append(entry.Files, id) return nil }); err != nil { - return 0, err + return nil, err } - return totalSize, nil + return entry, nil } // clean runs background cleaning of this cache until the process exits. // Returns the total size of the cache after it's finished. +// Sizes only count files that are not hard linked from outside the cache; those are typically also in someone's +// plz-out, and so removing them would not free any space (and would make it less likely for that file to be shared +// with the next repo that builds it). func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 { - entries := []cacheEntry{} + entries := []*cacheEntry{} + files := map[fileID]*cacheFile{} var totalSize uint64 if err := fs.Walk(cache.Dir, func(path string, isDir bool) error { - name := filepath.Base(path) - if cache.shouldClean(name, isDir) { - if size, marked := cache.isMarked(path); marked { - totalSize += size - if !cache.Compress { - return filepath.SkipDir // Already handled - } - return nil // Need to keep walking if we are dealing with compressed files - } - size, err := findSize(path) - if err != nil { - return err - } - info, err := os.Stat(path) - if err != nil { - return err - } - entries = append(entries, cacheEntry{ - Path: path, - Size: size, - Atime: atime.Get(info).Unix(), - }) + if !cache.shouldClean(filepath.Base(path), isDir) { + return nil // nothing particularly to do for other entries + } + if size, marked := cache.isMarked(path); marked { totalSize += size - if !cache.Compress { - return filepath.SkipDir - } + } else if entry, err := readEntry(path, files); err != nil { + // This can happen if another process is cleaning concurrently; it's not fatal to us. + log.Warning("Failed to read cache entry %s: %s", path, err) + } else { + entries = append(entries, entry) + } + if !cache.Compress { + return filepath.SkipDir // Already handled } - return nil // nothing particularly to do for other entries + return nil // Need to keep walking if we are dealing with compressed files }); err != nil { log.Error("error walking cache directory: %s\n", err) return totalSize } + for _, f := range files { + if f.Unshared = f.Links <= f.Refs; f.Unshared { + totalSize += f.Size + } + } + // Entries with no unshared files would free nothing if removed, so are never candidates for cleaning. + candidates := entries[:0] + for _, entry := range entries { + for _, id := range entry.Files { + if f := files[id]; f.Unshared { + entry.Size += f.Size + } + } + if entry.Size > 0 { + candidates = append(candidates, entry) + } + } log.Info("Total cache size: %s", humanize.Bytes(totalSize)) if totalSize < highWaterMark { return totalSize // Nothing to do, cache is small enough. } // OK, we need to slim it down a bit. We implement a simple LRU algorithm. - sort.Slice(entries, func(i, j int) bool { - diff := entries[i].Atime - entries[j].Atime + sort.Slice(candidates, func(i, j int) bool { + diff := candidates[i].Mtime - candidates[j].Mtime if diff > -accessTimeGracePeriod && diff < accessTimeGracePeriod { - return entries[i].Size > entries[j].Size + return candidates[i].Size > candidates[j].Size } - return entries[i].Atime < entries[j].Atime + return candidates[i].Mtime < candidates[j].Mtime }) - for _, entry := range entries { + for _, entry := range candidates { if _, marked := cache.isMarked(entry.Path); marked { continue } - log.Debug("Cleaning %s, accessed %s, saves %s", entry.Path, humanize.Time(time.Unix(entry.Atime, 0)), humanize.Bytes(entry.Size)) + log.Debug("Cleaning %s, used %s, saves up to %s", entry.Path, humanize.Time(time.Unix(entry.Mtime, 0)), humanize.Bytes(entry.Size)) if err := cache.cleanPath(entry.Path); err != nil { log.Warning("Error while cleaning cache: %s", err) continue } - totalSize -= entry.Size + // A file is only freed once the last entry referring to it is removed. + for _, id := range entry.Files { + f := files[id] + f.Refs-- + if f.Refs == 0 && f.Unshared { + totalSize -= f.Size + } + } if totalSize < lowWaterMark { break } @@ -470,6 +540,17 @@ func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 { return totalSize } +// touch updates the modification time of a cache entry to mark it as recently used. +func (cache *dirCache) touch(path string, info os.FileInfo) { + if time.Since(info.ModTime()) < touchInterval { + return // Updated recently enough already + } + now := time.Now() + if err := os.Chtimes(path, now, now); err != nil { + log.Debug("Failed to update modification time of %s: %s", path, err) + } +} + func (cache *dirCache) cleanPath(path string) error { lockFile, err := core.AcquireExclusiveFileLock(path + ".lock") if err != nil { diff --git a/src/cache/dir_cache_eviction.md b/src/cache/dir_cache_eviction.md new file mode 100644 index 0000000000..5b51dcea16 --- /dev/null +++ b/src/cache/dir_cache_eviction.md @@ -0,0 +1,189 @@ +# Dir cache eviction + +## Background + +Every repo (and every worktree of a repo) has its own `plz-out`, and these can get large. This +is getting worse with agentic workflows, which tend to create lots of worktrees. We can't share +`plz-out` itself between them, because repo-relative paths like `plz-out/gen/...` are baked +into commands, `$(location)` and so on. + +The dir cache already gives us most of the sharing we want. It's on by default at +`~/.cache/please`, so every repo on the machine shares it. It also hardlinks files both when +storing and when retrieving (`fs.RecursiveLink` in `dir_cache.go`). An output built in one +worktree and retrieved in another therefore ends up as a single inode on disk, linked from the +cache and from both `plz-outs`. + +The weak point is eviction, which is what this document is about. + +## Measurements + +We used `tools/misc/plz_out_usage.py`, which reports three totals: + +- *apparent*: the sum of each tree's own size. +- *actual*: bytes on disk, counting each inode once. +- *ideal*: one copy of each distinct file content. + +### Four worktrees building `//src/...` with a shared, initially empty cache + +| | apparent | actual | ideal | +|---|---|---|---| +| 1 worktree (master) | 1.82G | 1.54G | 1.40G | +| + master~5 | 3.63G | 2.09G | 1.95G | +| + master~20 | 5.42G | 2.64G | 2.50G | +| + master~50 (different Go version) | 7.17G | 4.11G | 3.56G | + +- Hardlinking through the cache already does most of the work: 7.2G apparent becomes 4.1G + actual. Without the cache it would be about 6G. +- A perfect content-addressed store would only save a further ~14%. That rules out building + one for now. +- Each extra worktree costs about 0.55G, even 5 commits apart. Almost all of that is content + that genuinely differs, mostly statically linked Go test binaries that relink whenever + `src/core` changes. Sharing whole files can't help with that. +- Filesystem compression, reflinks and copy-on-write can't be relied on. ext4 is the most + common filesystem for our users. + +### A real developer cache, after a few weeks of normal use + +- The cache was 8.2G against the 10G high water mark. 7.8G of that was hardlinked into some + `plz-out`, and only ~0.3G was held by the cache alone. +- About 29k entries had been written in the last day, but only 6.4k existed. The cache was + thrashing: roughly 22k entries were stored and evicted within a day. +- Every live entry had an atime under a day old. +- 25k of the 31.8k `.lock` files belonged to entries that no longer existed. + +## Problems with the original cleaner + +The original cleaner walked the whole cache at the start of every plz invocation. If the total +size was over the high water mark (10G), it evicted entries in order of directory atime until +the total was under the low water mark (8G). + +1. **Hardlinks aren't accounted for.** Evicting an entry that's also linked into a `plz-out` + frees nothing. It also breaks sharing: the next worktree that needs the entry rebuilds it + as a fresh, unshared copy. This is what caused the thrashing. +2. **The recency signal is broken.** With `relatime` (the usual default), atime only moves + about once a day. Retrieving an entry never lists its directory, but the cleaner's own walk + does. In effect the "LRU" order recorded when the cleaner last walked each entry. +3. **Every invocation cleans, with no coordination.** With several agents running at once, + each walks the cache and evicts against the same stale total, so they over-evict together. + A long-running process never cleans again. +4. **The fixed watermarks are hard to tune.** 10G is far too much on some machines and far + too little on others, and two thresholds is one more knob than anyone wants. +5. **Lock files leak.** Removing an entry leaves its `.lock` file behind. + +## Design + +Every rule below only counts, or deletes, files that are *unshared*: no hard link to them +exists outside the cache. The filesystem's link count does the reference counting for us. +When a worktree is deleted, the link counts of its files drop, and their cache entries become +evictable automatically. + +A file is unshared if its link count is no more than the number of links to it found inside +the cache during the walk. Comparing against the in-cache count, rather than 1, handles a +single file linked into more than one entry. For example, if both a target and a filegroup +that hardlinks its outputs are cached, the file is unshared once the two cache links are +the only ones left. + +### Policy + +Three rules decide what gets evicted. Each covers a weakness of the others. + +1. **Age (the main rule).** Evict entries that haven't been used for more than 7 days, + whatever the total size. Linked entries are never evicted, so this only affects entries + that no `plz-out` points at any more: outputs from deleted worktrees, or outputs that a + later rebuild replaced in the same worktree. The one real cost is that coming back to a + branch after more than a week means rebuilding instead of restoring. This matches what + similar tools do: Go's build cache uses 5 days, and Gradle and Cargo also age out + entries. +2. **A size cap relative to the disk.** Cap unshared bytes at 10% of the filesystem's total + size by default, using `unix.Statfs`. The low mark is derived from the cap (e.g. 80% of + it) rather than being a second setting. +3. **A free-space floor.** If free space on the filesystem drops below max(5% of the disk, + 5GB), evict unshared entries least-recently-used first until free space is back above the + floor or nothing evictable is left. If we run out, log a single warning saying that the + space is held by `plz-out` directories, not the cache, so `plz clean` in old worktrees is + the fix. + +Within the size and free-space rules, entries are evicted least-recently-used first. Entries +used within 10 minutes of each other are treated as equally recent, and the larger is evicted +first. + +### Recency + +Recency comes from the entry's mtime, not its atime: + +- A successful retrieve sets the mtime to now, but only if it's more than an hour old, so that + repeated retrieves don't cause constant writes. +- A store gives the entry a fresh mtime anyway. + +### Coordination + +- Only one process cleans at a time. It takes a non-blocking lock on `/.clean.lock`, + and any other process that can't get the lock skips cleaning. +- A timestamp file records when cleaning last ran. With the age rule doing most of the work, + a full clean about once a day is enough. Cleaning sooner is only needed when the size cap + or the free-space floor is breached. + +### Config + +- `DirCacheMaxAge` (new): defaults to 7 days. +- `DirCacheMinFreeSpace` (new): accepts bytes or a percentage. That needs a new config type, + because `cli.ByteSize` only takes bytes. +- `DirCacheHighWaterMark`: still honoured if set explicitly, overriding the percentage + default. It now measures unshared bytes only. +- `DirCacheLowWaterMark`: becomes optional. + +On small disks, such as CI runners, the percentage defaults give a smaller cache. They can +set explicit values if that matters. + +### Rejected options + +- **A machine-wide content-addressed store:** the measurements show only ~14% to gain over + hardlinking through the cache. +- **Reflinks, copy-on-write clones and filesystem compression:** ext4 doesn't support them. +- **Stripping binaries or keeping fewer test outputs:** out of scope. There will be reasons + people rely on the current behaviour. +- **Cost-aware eviction** (weighting entries by how long they took to build): extra + complexity for an unclear gain. +- **A persistent index instead of walking the cache:** the walk takes ~0.1s warm on 330k + files, so it isn't worth the risk of the index going stale or getting corrupted. +- **Free space as the only trigger:** it ties cache behaviour to unrelated disk usage, it + can't reclaim space held by `plz-out`, and it still needs a threshold to tune. + +## Implementation plan + +### PR 1: accounting and recency (done) + +- Count each entry's cost as the bytes in its unshared files. The total compared against + the watermarks is the sum of those costs, with each inode counted once. Entries with zero + cost are never evicted. +- When files are shared between cache entries, space is only treated as freed once the last + entry holding a file is removed. +- Bump the entry's mtime on retrieve, at most once an hour, and evict by mtime instead of + atime. This removes the `djherbis/atime` dependency. +- An entry that disappears mid-walk (e.g. another process cleaned it) is skipped instead of + aborting the whole walk. + +On the real cache above, the counted size drops from 8.2G to ~0.3G. + +### PR 2: policy + +- The 7-day age rule. +- The size cap as a percentage of the disk, with the low mark derived from it. +- The free-space floor, including the one-time warning. +- New config options, documentation in `docs/config.html`, and a ChangeLog entry. + +### PR 3: coordination and housekeeping + +- One cleaner at a time, using `/.clean.lock`. +- A timestamp file so cleaning doesn't run more often than needed. +- Delete an entry's `.lock` file along with it, and sweep orphaned lock files. This has to + allow for another process racing on the lock file. The existing rename-then-delete in + `cleanPath` means a racing retrieve just misses, harmlessly. + +### Possible later work + +- Stop copying the Go toolchain sources between `gen/third_party/go/std` and + `bin/third_party/go/toolchain`; hardlink them instead. +- Stop re-extracting the same third-party modules when a subrepo's key changes. + +Both are small wins found by the measurement script. diff --git a/src/cache/dir_cache_test.go b/src/cache/dir_cache_test.go index c3c39df98f..f1bb6c22df 100644 --- a/src/cache/dir_cache_test.go +++ b/src/cache/dir_cache_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" @@ -60,11 +61,9 @@ func TestStoreAndRetrieve(t *testing.T) { func TestCleanNoop(t *testing.T) { cache := makeCache(".plz-cache-test2", false) target1 := makeTarget2("//test2:target1", 2000) - cache.Store(target1, hash, target1.Outputs(nil)) - assert.True(t, inCache(target1)) + writeFile(cachePath(target1, false), 2000) target2 := makeTarget2("//test2:target2", 2000) - cache.Store(target2, hash, target2.Outputs(nil)) - assert.True(t, inCache(target2)) + writeFile(cachePath(target2, false), 2000) // Doesn't clean anything this time because the high water mark is sufficiently high totalSize := cache.clean(20000, 1000) assert.EqualValues(t, 12000, totalSize) @@ -80,10 +79,10 @@ func TestCleanNoop2(t *testing.T) { target2 := makeTarget2("//test3:target2", 2000) cache.Store(target2, hash, target2.Outputs(nil)) assert.True(t, inCache(target2)) - // Doesn't clean anything this time, the high water mark is lower but both targets have - // just been built. - totalSize := cache.clean(10000, 1000) - assert.EqualValues(t, 12000, totalSize) + // Doesn't clean anything this time; both targets have just been built, and are hard linked + // into plz-out so they don't count towards the size anyway. + totalSize := cache.clean(1, 0) + assert.EqualValues(t, 0, totalSize) assert.True(t, inCache(target1)) assert.True(t, inCache(target2)) } @@ -97,8 +96,8 @@ func TestCleanForReal(t *testing.T) { writeFile(cachePath(target2, false), 2000) assert.True(t, inCache(target2)) // This time it should clean target2, because target1 has just been stored - totalSize := cache.clean(10000, 1000) - assert.EqualValues(t, 6000, totalSize) + totalSize := cache.clean(5000, 1000) + assert.EqualValues(t, 0, totalSize) assert.True(t, inCache(target1)) assert.False(t, inCache(target2)) } @@ -112,12 +111,78 @@ func TestCleanForReal2(t *testing.T) { cache.Store(target2, hash, target2.Outputs(nil)) assert.True(t, inCache(target2)) // This time it should clean target1, because target2 has just been stored - totalSize := cache.clean(10000, 1000) - assert.EqualValues(t, 6000, totalSize) + totalSize := cache.clean(5000, 1000) + assert.EqualValues(t, 0, totalSize) assert.False(t, inCache(target1)) assert.True(t, inCache(target2)) } +func TestCleanSkipsLinkedEntries(t *testing.T) { + makeCache(".plz-cache-test8", false).Store(makeTarget2("//test8:target1", 2000), hash, []string{"test.go"}) + target1 := makeTarget2("//test8:target1", 2000) + cache := makeCache(".plz-cache-test8", false) // A new instance doesn't know what the previous one stored. + // Hardlink the file in again, as a second build would do on retrieving it. + out := filepath.Join("plz-out/gen", target1.Label.PackageName, "test.go") + assert.NoError(t, os.Remove(out)) + assert.NoError(t, os.Link(cachePath(target1, false), out)) + // The entry is still linked into plz-out so cleaning it won't save anything. + assert.EqualValues(t, 0, cache.clean(1, 0)) + assert.True(t, inCache(target1)) + // Once that link is gone (e.g. the repo is deleted), it does count and can be cleaned. + assert.NoError(t, os.Remove(out)) + assert.EqualValues(t, 0, cache.clean(1000, 0)) + assert.False(t, inCache(target1)) +} + +func TestCleanSharedWithinCache(t *testing.T) { + cache := makeCache(".plz-cache-test9", false) + target1 := makeTarget2("//test9:target1", 2000) + writeFile(cachePath(target1, false), 2000) + target2 := makeTarget2("//test9:target2", 2000) + assert.NoError(t, os.MkdirAll(filepath.Dir(cachePath(target2, false)), core.DirPermissions)) + assert.NoError(t, os.Link(cachePath(target1, false), cachePath(target2, false))) + setMtime(t, cachePath(target1, false), 2*time.Hour) + // The file is only linked within the cache, so it counts but only once. + assert.EqualValues(t, 6000, cache.clean(10000, 0)) + // Removing the first entry doesn't free anything, so it has to remove the second too. + assert.EqualValues(t, 0, cache.clean(5000, 1000)) + assert.False(t, inCache(target1)) + assert.False(t, inCache(target2)) +} + +func TestCleanLeastRecentlyUsed(t *testing.T) { + cache := makeCache(".plz-cache-test10", false) + target1 := makeTarget2("//test10:target1", 2000) + writeFile(cachePath(target1, false), 2000) + setMtime(t, cachePath(target1, false), time.Hour) + target2 := makeTarget2("//test10:target2", 2000) + writeFile(cachePath(target2, false), 2000) + setMtime(t, cachePath(target2, false), 3*time.Hour) + target3 := makeTarget2("//test10:target3", 2000) + writeFile(cachePath(target3, false), 2000) + setMtime(t, cachePath(target3, false), 2*time.Hour) + // Should remove the least recently used entry first, which is target2. + assert.EqualValues(t, 12000, cache.clean(15000, 13000)) + assert.True(t, inCache(target1)) + assert.False(t, inCache(target2)) + assert.True(t, inCache(target3)) +} + +func TestRetrieveUpdatesMtime(t *testing.T) { + cache := makeCache(".plz-cache-test11", false) + target := makeTarget2("//test11:target1", 20) + cache.Store(target, hash, target.Outputs(nil)) + entry := filepath.Dir(cachePath(target, false)) + // Not updated if it was recently modified + setMtime(t, cachePath(target, false), 30*time.Minute) + assert.True(t, cache.Retrieve(target, hash, target.Outputs(nil))) + assert.True(t, time.Since(getMtime(t, entry)) > 20*time.Minute) + // But it is updated if it's older + setMtime(t, cachePath(target, false), 2*time.Hour) + assert.True(t, cache.Retrieve(target, hash, target.Outputs(nil))) + assert.True(t, time.Since(getMtime(t, entry)) < time.Minute) +} + func TestStoreAndRetrieveCompressed(t *testing.T) { cache := makeCache(".plz-cache-test6", true) target := makeTarget2("//test6:target6", 20) @@ -147,6 +212,20 @@ func TestCleanCompressed(t *testing.T) { assert.True(t, inCompressedCache(target2)) } +// setMtime sets the modification time of the cache entry containing the given file to the given time ago. +func setMtime(t *testing.T, filename string, ago time.Duration) { + t.Helper() + then := time.Now().Add(-ago) + assert.NoError(t, os.Chtimes(filepath.Dir(filename), then, then)) +} + +func getMtime(t *testing.T, filename string) time.Time { + t.Helper() + info, err := os.Stat(filename) + assert.NoError(t, err) + return info.ModTime() +} + func makeCache(dir string, compress bool) *dirCache { config := core.DefaultConfiguration() config.Cache.Dir = dir diff --git a/third_party/go/BUILD b/third_party/go/BUILD index 7fbb6b9cad..9284689e60 100644 --- a/third_party/go/BUILD +++ b/third_party/go/BUILD @@ -164,11 +164,6 @@ go_repo( version = "v0.0.0-20260706201446-f0a921348800", ) -go_repo( - module = "github.com/djherbis/atime", - version = "v1.1.0", -) - go_repo( module = "github.com/hashicorp/go-retryablehttp", version = "v0.7.8", diff --git a/tools/misc/BUILD b/tools/misc/BUILD index 2d2dccebfc..5b04df98c8 100644 --- a/tools/misc/BUILD +++ b/tools/misc/BUILD @@ -28,3 +28,14 @@ sh_binary( name = "lint", main = "lint.sh", ) + +python_binary( + name = "plz_out_usage", + main = "plz_out_usage.py", +) + +python_test( + name = "plz_out_usage_test", + srcs = ["plz_out_usage_test.py"], + deps = [":plz_out_usage"], +) From 56944999c2f6d08875fe570713eac52b7f4e2973 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Fri, 9 Oct 2026 17:21:45 +0100 Subject: [PATCH 2/2] revert --- tools/misc/BUILD | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tools/misc/BUILD b/tools/misc/BUILD index 5b04df98c8..2d2dccebfc 100644 --- a/tools/misc/BUILD +++ b/tools/misc/BUILD @@ -28,14 +28,3 @@ sh_binary( name = "lint", main = "lint.sh", ) - -python_binary( - name = "plz_out_usage", - main = "plz_out_usage.py", -) - -python_test( - name = "plz_out_usage_test", - srcs = ["plz_out_usage_test.py"], - deps = [":plz_out_usage"], -)