siso: keep the hashfs entry when RemoveAll fails on disk When clean-dead deletes a dead output and the disk removal fails, for example due to a transient sharing violation on Windows, RemoveAll still replaced the in-memory entry with one whose err field holds the removal error. State() skips entries with a non-nil err, so the build saved its state without that path. The next build derives prevGenerated only from loaded state entries, so the file never appeared in PreviouslyGeneratedFiles again and clean-dead never retried the removal. One transient error was enough to exempt a dead file from cleanup forever. A failed RemoveAll always leaves the root path on disk. So in that case, keep the recorded entry (still invalidating the dir-input cache) and store the not-exist entry only when the disk removal succeeded. The kept root entry keeps its cmdhash, so the next build's clean-dead retries the removal. Children need the opposite treatment: a failed RemoveAll may already have deleted some of them, and those must not stay recorded. CleanDead only warns and continues into the build, and a stale child entry can satisfy Stat for a path that is gone from disk. If the manifest meanwhile turned the directory output into a direct member output, an unchanged command would be judged up to date and the build could finish with the output missing. So drop every recorded child that Lstat proves absent, and keep everything that is still on disk. Change-Id: Ib88d5a5dd9654773a3ff554d9023af6bbb73c691 Reviewed-on: https://chromium-review.googlesource.com/c/build/+/8037824 Reviewed-by: Yuke Liao <liaoyuke@google.com> Auto-Submit: Philipp Wollermann <philwo@google.com> Commit-Queue: Neri Marschik <nerima@google.com>
diff --git a/siso/build/ninjabuild/ninjabuild_test.go b/siso/build/ninjabuild/ninjabuild_test.go index acb0473..c043db6 100644 --- a/siso/build/ninjabuild/ninjabuild_test.go +++ b/siso/build/ninjabuild/ninjabuild_test.go
@@ -329,6 +329,15 @@ if _, err := os.Lstat(filepath.Join(outDir, "gen/dead2.o")); !os.IsNotExist(err) { t.Errorf("Lstat(gen/dead2.o) err=%v; want not-exist (the deletable dead file is still removed)", err) } + // The undeletable file must survive in saved state, so it re-enters + // PreviouslyGeneratedFiles and the next build's clean-dead retries it. + m := hashfs.StateMap(hashFS.DigestFunction(), hashFS.State(ctx)) + if _, ok := m[filepath.ToSlash(filepath.Join(outDir, "dead.o"))]; !ok { + t.Errorf("dead.o missing from saved state; the next build's clean-dead will never retry removing it") + } + if _, ok := m[filepath.ToSlash(filepath.Join(outDir, "gen/dead2.o"))]; ok { + t.Errorf("gen/dead2.o still in saved state; want dropped (it was removed)") + } } func TestTargets(t *testing.T) {
diff --git a/siso/hashfs/fs.go b/siso/hashfs/fs.go index a2c439e..c9423d6 100644 --- a/siso/hashfs/fs.go +++ b/siso/hashfs/fs.go
@@ -1120,29 +1120,61 @@ } }) // Route through OSFS.RemoveAll for its metrics and slow-operation logging. - removeErr := hfs.OS.RemoveAll(ctx, string(name)) - // The in-memory entry records the post-state: the path no longer exists. - // This is distinct from what we return to the caller: a failed on-disk - // removal must surface, not be masked by the entry bookkeeping below. - entryErr := removeErr - if entryErr == nil { - entryErr = fs.ErrNotExist + if err := hfs.OS.RemoveAll(ctx, string(name)); err != nil { + // A failed RemoveAll always leaves the root on disk, so keep the + // recorded entry: dropping it would drop the path from saved state, + // and clean-dead would never retry. Children RemoveAll did delete + // must go: a stale entry would satisfy Stat for a missing path. + hfs.dropRemovedChildren(ctx, name) + hfs.invalidateDirInputCache(string(name)) + clog.Warningf(ctx, "removeAll %q: %v", name, err) + return err } lready := make(chan bool, 1) lready <- true e := &entry{ lready: lready, - err: entryErr, + err: fs.ErrNotExist, } _, storeErr := hfs.directory.store(ctx, name, e) hfs.invalidateDirInputCache(string(name)) - clog.Infof(ctx, "removeAll %s: %v", name, removeErr) - if removeErr != nil { - return removeErr - } + clog.Infof(ctx, "removeAll %q", name) return storeErr } +// dropRemovedChildren drops recorded entries under name whose on-disk path a +// partially failed RemoveAll already deleted. Only a path proven absent by +// Lstat is dropped (with its whole subtree); anything still on disk, or that +// cannot be checked, stays recorded. +func (hfs *HashFS) dropRemovedChildren(ctx context.Context, name path.Path) { + e, _, _, ok := hfs.directory.lookup(ctx, name) + if !ok || e == nil { + return + } + hfs.dropRemovedChildrenIn(ctx, name, e.getDir()) +} + +func (hfs *HashFS) dropRemovedChildrenIn(ctx context.Context, dirname path.Path, d *directory) { + if d == nil { + return + } + d.m.Range(func(k, v any) bool { + base := k.(string) + child := v.(*entry) + fname := dirname.Join(base) + _, err := hfs.OS.Lstat(ctx, string(fname)) + switch { + case errors.Is(err, fs.ErrNotExist): + // Use CompareAndDelete so a concurrent re-store is never clobbered. + d.m.CompareAndDelete(base, child) + clog.Infof(ctx, "removeAll: drop removed child %q", fname) + case err == nil: + hfs.dropRemovedChildrenIn(ctx, fname, child.getDir()) + } + return true + }) +} + // ClearStaleFileForDirOutput removes name (and its hashfs subtree) when a // previous build left a non-directory where a directory output is now // declared, so the action can create its directory there.
diff --git a/siso/hashfs/removeall_state_test.go b/siso/hashfs/removeall_state_test.go new file mode 100644 index 0000000..69aea07 --- /dev/null +++ b/siso/hashfs/removeall_state_test.go
@@ -0,0 +1,173 @@ +// Copyright 2026 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +package hashfs_test + +import ( + "bytes" + "errors" + "io/fs" + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "go.chromium.org/build/hashigo/digest" + + "go.chromium.org/build/siso/hashfs" + "go.chromium.org/build/siso/path" +) + +// TestState_RemoveAllFailureKeepsEntry verifies a failed disk removal keeps +// the generated file's hashfs entry. The file then stays in saved state and +// in PreviouslyGeneratedFiles, so the next build's clean-dead retries the +// removal instead of leaving the dead output behind forever. +func TestState_RemoveAllFailureKeepsEntry(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("directory permission bits do not block removal on windows") + } + if os.Getuid() == 0 { + t.Skip("running as root; permission bits are not enforced") + } + ctx := t.Context() + dir := t.TempDir() + dir, err := filepath.EvalSymlinks(dir) + if err != nil { + t.Fatal(err) + } + hfs, err := hashfs.New(ctx, hashfs.Option{}) + if err != nil { + t.Fatal(err) + } + defer hfs.Close(ctx) + if err := hfs.WaitReady(ctx); err != nil { + t.Fatal(err) + } + + cmdhash := []byte("dummy-cmdhash") + if err := hfs.WriteFile(ctx, dir, "gen/dead", []byte("dead"), false, time.Now(), cmdhash, nil); err != nil { + t.Fatalf("WriteFile=%v", err) + } + if err := hfs.Flush(ctx, dir, []path.Path{"gen/dead"}); err != nil { + t.Fatalf("Flush=%v", err) + } + + // Strip write permission from the parent so unlinking gen/dead fails, + // like a sharing violation or permission error during clean-dead. + genDir := filepath.Join(dir, "gen") + if err := os.Chmod(genDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := os.Chmod(genDir, 0o755); err != nil { + t.Error(err) + } + }) + + if err := hfs.RemoveAll(ctx, dir, "gen/dead"); err == nil { + t.Fatal("RemoveAll=nil; want error (test setup failed to block the removal)") + } + if _, err := os.Lstat(filepath.Join(dir, "gen/dead")); err != nil { + t.Fatalf("Lstat(gen/dead)=%v; want file left on disk", err) + } + + m := hashfs.StateMap(digest.SHA256, hfs.State(ctx)) + ent, ok := m[filepath.ToSlash(filepath.Join(dir, "gen/dead"))] + if !ok { + t.Fatal("gen/dead missing from state after failed RemoveAll; clean-dead will never retry removing it") + } + if !bytes.Equal(ent.CmdHash, cmdhash) { + t.Errorf("gen/dead cmdhash = %q; want %q so it stays in PreviouslyGeneratedFiles", ent.CmdHash, cmdhash) + } +} + +// TestState_RemoveAllPartialFailureDropsRemovedChildren covers a RemoveAll +// that deletes some members of a directory before failing on another (e.g. a +// sibling locked by a scanner on Windows). The root entry must stay recorded +// so clean-dead retries the removal; entries for members proven gone from +// disk must be dropped, or a stale entry satisfies Stat for a missing path. +func TestState_RemoveAllPartialFailureDropsRemovedChildren(t *testing.T) { + if runtime.GOOS != "windows" && os.Getuid() == 0 { + t.Skip("running as root; permission bits are not enforced") + } + ctx := t.Context() + dir := t.TempDir() + dir, err := filepath.EvalSymlinks(dir) + if err != nil { + t.Fatal(err) + } + hfs, err := hashfs.New(ctx, hashfs.Option{}) + if err != nil { + t.Fatal(err) + } + defer hfs.Close(ctx) + if err := hfs.WaitReady(ctx); err != nil { + t.Fatal(err) + } + + cmdhash := []byte("dummy-cmdhash") + if err := hfs.Mkdir(ctx, dir, "gen/dead", cmdhash, nil); err != nil { + t.Fatalf("Mkdir=%v", err) + } + for _, f := range []path.Path{"gen/dead/a", "gen/dead/sub/f"} { + if err := hfs.WriteFile(ctx, dir, f, []byte("dead"), false, time.Now(), cmdhash, nil); err != nil { + t.Fatalf("WriteFile(%q)=%v", f, err) + } + } + if err := hfs.Flush(ctx, dir, []path.Path{"gen/dead/a", "gen/dead/sub/f"}); err != nil { + t.Fatalf("Flush=%v", err) + } + + // Block removal of gen/dead/sub/f while gen/dead/a stays deletable. + if runtime.GOOS == "windows" { + // An open handle without FILE_SHARE_DELETE makes unlinking fail with a + // sharing violation, the common cause in the wild. + fh, err := os.Open(filepath.Join(dir, "gen/dead/sub/f")) + if err != nil { + t.Fatal(err) + } + defer fh.Close() + } else { + subDir := filepath.Join(dir, "gen/dead/sub") + if err := os.Chmod(subDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := os.Chmod(subDir, 0o755); err != nil { + t.Error(err) + } + }) + } + + if err := hfs.RemoveAll(ctx, dir, "gen/dead"); err == nil { + t.Fatal("RemoveAll=nil; want error (test setup failed to block the removal)") + } + // os.RemoveAll continues past the member it cannot delete, so gen/dead/a + // must be gone from disk while gen/dead/sub/f survived. + if _, err := os.Lstat(filepath.Join(dir, "gen/dead/a")); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("Lstat(gen/dead/a)=%v; want ErrNotExist (test setup: RemoveAll did not delete it)", err) + } + if _, err := os.Lstat(filepath.Join(dir, "gen/dead/sub/f")); err != nil { + t.Fatalf("Lstat(gen/dead/sub/f)=%v; want blocked member left on disk", err) + } + + // The deleted member must not be statable from a stale entry. + if _, err := hfs.Stat(ctx, dir, "gen/dead/a"); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("Stat(gen/dead/a)=%v; want ErrNotExist (deleted from disk by RemoveAll)", err) + } + + // The root and the surviving member stay recorded so the next build's + // clean-dead retries the removal. + m := hashfs.StateMap(digest.SHA256, hfs.State(ctx)) + if _, ok := m[filepath.ToSlash(filepath.Join(dir, "gen/dead"))]; !ok { + t.Error("gen/dead missing from state after failed RemoveAll; clean-dead will never retry removing it") + } + if _, ok := m[filepath.ToSlash(filepath.Join(dir, "gen/dead/sub/f"))]; !ok { + t.Error("gen/dead/sub/f missing from state; it is still on disk and must stay recorded") + } + if _, ok := m[filepath.ToSlash(filepath.Join(dir, "gen/dead/a"))]; ok { + t.Error("gen/dead/a still in state; its stale entry must be dropped after RemoveAll deleted it from disk") + } +}