Repository navigation
Implement locking & co-ordination between cache cleans - #3644
Open
peterebden wants to merge 17 commits into
Open
peterebden wants to merge 17 commits into
peterebden wants to merge 17 commits into
Conversation
Couple of small things where we're over-downloading targets. Also maintain ordering of original target list for output.
Moves a bunch of stuff to `slices.SortFunc` instead of `sort.Slice`. It's generally a bit faster (per the docs) and we drop several types that existed only to implement `sort.Interface`.
I didn't realise this would be such a difficult change, but it's caused several issues so far. It seems like it's more trouble than it's worth so I'm putting back the old ordering.
Save some worries about potential deadlocks if these functions don't have that limitation any more. We don't call these functions on a hot path so allocating one slice per shard shouldn't cost much.
Related to #3449. `BuildState.Initialise` reads the subrepo's .plzconfig inside a sync.Once, but the error only went back to whichever call happened to run the Once. Any later call for the same subrepo got nil and carried on with a half-read config. Easy to see by building two targets from a subrepo whose .plzconfig doesn't parse. On master only one of them fails, the other gets parsed as if the config were fine. With this change both fail with the config error. I store the result on the state so every caller gets it, and reset it in Copy() next to initOnce. Added a small test in src/core/subrepo_test.go that fails on master. Not sure this covers everything in the issue (the arch subrepo case there was against older code), so I didn't mark it as a fix. Ran `plz test //src/core:core_test //src/plz/... //src/parse/...` and `//test/subrepo/...`, all passing.
Main thing is getting `plz init plugin go` to just write the filegroup and set `modfile`, which should avoid the main Puku papercut where it just silently does nothing (that behaviour also needs a fix in Puku really, it exits with 0 in too many unsuccessful cases). Also some minor changes to the codelab as I ran through it: - Remove the version pin. It gets out of date and the codelab should be happy to just use the latest. - Don't label the puku alias as optional, because the rest of the codelab references `plz puku` so it won't work without. (We might also want `plz init plugin go` to just do this?) - Remove the `go install std` directive. We don't need it any more (we build it via plz on-demand) - Remove the initial "sync the changes" bit; it doesn't sync at that point for me. I think you need to run `plz puku fmt` first. Fixes #3534
This was a little lazy, to reinvent a `context.Background()` in the middle of things. This plumbs it through so it doesn't need to get a new one. There's a potential edge case involving one of these being needed for a preload and triggering more parses which now lack the correct context info. It seems fairly niche but possible that such a thing might happen somewhere (hyrum's law etc).
We've been running this for some days now and it all seems fine. If we find further bugs we'll have to do patch releases for them.
jwong-beep
reviewed
Oct 28, 2026
| func (cache *dirCache) Store(target *core.BuildTarget, key []byte, files []string) { | ||
| cacheDir := cache.getPath(target, key, "") | ||
| lockFile, err := core.AcquireExclusiveFileLock(cacheDir + ".lock") | ||
| lockFile, err := lockEntry(cacheDir, syscall.LOCK_EX) |
Contributor
There was a problem hiding this comment.
Am I right that cacheDir sometimes isn't actually a directory but a file? I was a little confused by why you are writing a lock file under it but then delete the entire directory at line 48
| func tryLock(path string, flag int) *os.File { | ||
| f, err := os.OpenFile(path, os.O_RDWR|flag, 0644) | ||
| if err != nil { | ||
| if !os.IsNotExist(err) { |
Contributor
There was a problem hiding this comment.
Nit: Should we use errors.Is now or do we still want to support very old Go versions?
| } | ||
| cache.clean(policy, freeSpace) | ||
| now := time.Now() | ||
| if err := os.WriteFile(timestampFile, nil, 0644); err != nil { |
Contributor
There was a problem hiding this comment.
Should we write the timestamp into the file regardless? It's not using the timestamp in the name, it may be unintuitive to find out how to read the timestamp. Everyone knows how to use ls and cat, not everyone knows how to use stat.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third and final part of #3629 , stacked on #3642
Implements a lockfile for the cache so if a clean is in progress, another process will see that and will not try to do it as well.
Also implements cleaning of stale lock files across the cache so they don't build up forever.