From a0464d7e36e611addaac5a9d381424c2595e6237 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Apr 2026 16:24:16 -0700 Subject: [PATCH] Fix macOS tmpdir cleanup race caused by system daemons creating ~/Library On macOS, system daemons (cfprefsd, lsd) automatically create $HOME/Library/ for any process whose HOME they observe. Since the build env sets HOME=tmpDir, these daemons can recreate Library/ inside a target's temporary directory during or just after cleanup, causing os.RemoveAll to fail with ENOTEMPTY. Handle this with a platform-specific cleanTmpDir that detects the Library/ directory on ENOTEMPTY and retries removal with bounded backoff. Non-Darwin platforms get a zero-overhead passthrough. --- src/build/BUILD | 2 ++ src/build/build_step.go | 33 +++++++++++++++-- src/build/clean_tmpdir_darwin.go | 62 ++++++++++++++++++++++++++++++++ src/build/clean_tmpdir_other.go | 11 ++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 src/build/clean_tmpdir_darwin.go create mode 100644 src/build/clean_tmpdir_other.go diff --git a/src/build/BUILD b/src/build/BUILD index 6ec3b583a..5a3084569 100644 --- a/src/build/BUILD +++ b/src/build/BUILD @@ -2,6 +2,8 @@ go_library( name = "build", srcs = [ "build_step.go", + "clean_tmpdir_darwin.go", + "clean_tmpdir_other.go", "filegroup.go", "incrementality.go", ], diff --git a/src/build/build_step.go b/src/build/build_step.go index 303c75097..8fdbf0255 100644 --- a/src/build/build_step.go +++ b/src/build/build_step.go @@ -407,8 +407,10 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b } // Clean up the temporary directory once it's done. if state.CleanWorkdirs { - if err := fs.RemoveAll(target.TmpDir()); err != nil { - log.Warning("Failed to remove temporary directory for %s: %s", target.Label, err) + tmpDir := target.TmpDir() + if err := cleanTmpDir(tmpDir); err != nil { + log.Warning("Failed to remove temporary directory for %q: %v", target.Label, err) + logStaleDirectoryContents(tmpDir) } } if outputsChanged { @@ -1231,3 +1233,30 @@ func build(state *core.BuildState, target *core.BuildTarget, inputHash []byte) ( } return nil, fmt.Errorf("Persistent workers are no longer supported, found worker command: %s", workerCmd) } + +// logStaleDirectoryContents recursively logs the remaining contents of a +// directory that os.RemoveAll failed to remove. This helps diagnose what is +// still holding files open or creating files during cleanup. +func logStaleDirectoryContents(dir string) { + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil { + log.Debug(" stale (walk error): %s: %v", path, err) + return nil + } + if path == dir { + return nil + } + rel, _ := filepath.Rel(dir, path) + info, ierr := d.Info() + if ierr != nil { + log.Debug(" stale: %s (stat failed: %v)", rel, ierr) + return nil + } + log.Debug(" stale: %s (type=%s, size=%d, mtime=%s)", + rel, d.Type(), info.Size(), info.ModTime().Format("15:04:05.000")) + return nil + }) + if err != nil { + log.Debug(" could not walk remaining contents of %s: %v", dir, err) + } +} diff --git a/src/build/clean_tmpdir_darwin.go b/src/build/clean_tmpdir_darwin.go new file mode 100644 index 000000000..d7fca8b8b --- /dev/null +++ b/src/build/clean_tmpdir_darwin.go @@ -0,0 +1,62 @@ +package build + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "syscall" + "time" + + "github.com/thought-machine/please/src/fs" +) + +// cleanTmpDir removes a target's temporary build directory. +// +// On macOS, the operating system automatically creates a ~/Library directory +// structure for every HOME directory it observes in use by a process. This is +// documented in Apple's File System Programming Guide: +// +// https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/MacOSXDirectories/MacOSXDirectories.html +// +// Several per-user system daemons (cfprefsd, lsd, etc.) monitor process HOME +// values and lazily create $HOME/Library/ and its subdirectories (Preferences, +// Caches, etc.) for any new HOME path they observe. This creation is +// asynchronous and can occur after the process that triggered it has already +// exited. +// +// Since the build environment sets HOME=tmpDir (to isolate each target's build +// from the user's home directory), these daemons may create a Library/ +// directory inside the target's tmpDir during or shortly after the build +// completes. When os.RemoveAll deletes the tmpDir contents and then attempts +// to remove the now-empty directory, a daemon may have re-created Library/ in +// the interim, causing the removal to fail with ENOTEMPTY. +// +// We handle this by detecting the Library/ directory after an ENOTEMPTY +// failure, removing it, and retrying with bounded backoff to allow the daemons +// to settle. +func cleanTmpDir(tmpDir string) error { + err := fs.RemoveAll(tmpDir) + if err == nil || !errors.Is(err, syscall.ENOTEMPTY) { + return err + } + + libDir := filepath.Join(tmpDir, "Library") + const maxAttempts = 3 + for attempt := range maxAttempts { + if attempt > 0 { + time.Sleep(time.Second) + } + if info, serr := os.Stat(libDir); serr != nil || !info.IsDir() { + // Library/ is not the problem; return the original error. + return err + } + os.RemoveAll(libDir) + if rerr := os.Remove(tmpDir); rerr == nil { + return nil + } else if !errors.Is(rerr, syscall.ENOTEMPTY) { + return rerr + } + } + return fmt.Errorf("failed to remove %s after %d attempts to clean macOS Library dir: %w", tmpDir, maxAttempts, err) +} diff --git a/src/build/clean_tmpdir_other.go b/src/build/clean_tmpdir_other.go new file mode 100644 index 000000000..638103cea --- /dev/null +++ b/src/build/clean_tmpdir_other.go @@ -0,0 +1,11 @@ +//go:build !darwin +// +build !darwin + +package build + +import "github.com/thought-machine/please/src/fs" + +// cleanTmpDir removes a target's temporary build directory. +func cleanTmpDir(tmpDir string) error { + return fs.RemoveAll(tmpDir) +}