From 49979c4da4fd9e2c4b411e50af3c9418dd416a9f Mon Sep 17 00:00:00 2001 From: Mikhail Chusavitin Date: Tue, 28 Jul 2026 16:28:11 +0300 Subject: [PATCH] app: don't let a renamed-away source path abort the whole blackbox/support-bundle copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copyPath/copyPathFiltered treated any os.Stat/os.ReadDir/os.Open error on a source entry as fatal, aborting the entire tree copy. A source path is read from the live export dir while bee's own task runner concurrently renames task directories (e.g. "_pending" -> "_done") — an entry present in the parent's os.ReadDir a moment ago disappearing by the time it's individually Stat'd/Open'd is an expected race, not a real failure. Seen on a real crash bundle: blackbox got stuck in status "degraded" from early in the run (first hit during the CPU pack, well before the GPU tests) after exactly this race, and every syncBracket wait then timed out for the rest of the run — the discovery/wait plumbing from the previous fix works, but had nothing working under it to wait on. The target also accumulated stale "_pending" copies alongside "_done" ones with no cleanup, though fixing that dedup is left for a follow-up. os.IsNotExist(err) now skips the vanished entry instead of propagating. Added regression tests simulating the race directly (copy_path_race_test.go). Co-Authored-By: Claude Sonnet 5 --- audit/internal/app/copy_path_race_test.go | 100 ++++++++++++++++++++++ audit/internal/app/support_bundle.go | 26 ++++++ 2 files changed, 126 insertions(+) create mode 100644 audit/internal/app/copy_path_race_test.go diff --git a/audit/internal/app/copy_path_race_test.go b/audit/internal/app/copy_path_race_test.go new file mode 100644 index 0000000..ba8968c --- /dev/null +++ b/audit/internal/app/copy_path_race_test.go @@ -0,0 +1,100 @@ +package app + +import ( + "os" + "path/filepath" + "testing" +) + +// These simulate the race that left blackbox stuck "degraded" on a real +// crash bundle: bee's own task runner renames a task's artifacts dir +// (e.g. "_pending" -> "_done") concurrently with a copy walking the export +// tree. A source entry that existed when its parent directory was listed but +// is gone by the time it's individually Stat'd/Open'd must be skipped, not +// treated as a fatal error that aborts the whole sync cycle. + +func TestCopyPathSkipsEntryRemovedBetweenListingAndStat(t *testing.T) { + srcDir := t.TempDir() + dstDir := t.TempDir() + + present := filepath.Join(srcDir, "present.txt") + if err := os.WriteFile(present, []byte("data"), 0644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + vanished := filepath.Join(srcDir, "vanished.txt") + + // Never actually create "vanished.txt": copyPath is called directly on + // its path, standing in for "listed by a parent os.ReadDir a moment ago, + // gone by the time we get here" without needing real concurrency. + if err := copyPath(vanished, filepath.Join(dstDir, "vanished.txt")); err != nil { + t.Fatalf("copyPath on a vanished entry should be skipped, not fail: %v", err) + } + if err := copyPath(present, filepath.Join(dstDir, "present.txt")); err != nil { + t.Fatalf("copyPath on a present entry: %v", err) + } + if _, err := os.Stat(filepath.Join(dstDir, "present.txt")); err != nil { + t.Fatalf("present.txt should have been copied: %v", err) + } + if _, err := os.Stat(filepath.Join(dstDir, "vanished.txt")); !os.IsNotExist(err) { + t.Fatalf("vanished.txt should not exist in dst, got err=%v", err) + } +} + +func TestCopyPathSkipsDirRemovedAfterMkdir(t *testing.T) { + // A directory that disappears between copyPath creating dst and reading + // src's entries (e.g. renamed away mid-copy) must not fail the whole + // tree copy either. + srcDir := t.TempDir() + dstDir := t.TempDir() + + renamedAway := filepath.Join(srcDir, "007_pending") + if err := os.MkdirAll(renamedAway, 0755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + // Remove it before copyPath's own os.Stat, same effect as another + // goroutine renaming it out from under the copy. + if err := os.RemoveAll(renamedAway); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + if err := copyPath(renamedAway, filepath.Join(dstDir, "007_pending")); err != nil { + t.Fatalf("copyPath on a vanished dir should be skipped, not fail: %v", err) + } +} + +func TestCopyPathFilteredSkipsEntryRemovedBetweenListingAndStat(t *testing.T) { + srcDir := t.TempDir() + dstDir := t.TempDir() + vanished := filepath.Join(srcDir, "vanished.txt") + + keepAll := func(string, os.FileInfo) bool { return true } + if err := copyPathFiltered(srcDir, vanished, filepath.Join(dstDir, "vanished.txt"), keepAll); err != nil { + t.Fatalf("copyPathFiltered on a vanished entry should be skipped, not fail: %v", err) + } +} + +func TestCopyDirContentsFilteredToleratesEntryRemovedMidWalk(t *testing.T) { + srcDir := t.TempDir() + dstDir := t.TempDir() + + if err := os.WriteFile(filepath.Join(srcDir, "a.txt"), []byte("a"), 0644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + staleDir := filepath.Join(srcDir, "007_nvbandwidth_pending") + if err := os.MkdirAll(staleDir, 0755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + // Simulate the rename race by removing the dir right before the copy + // would reach it — copyDirContentsFiltered must finish copying the rest + // of the tree (a.txt) rather than aborting on the missing entry. + if err := os.RemoveAll(staleDir); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + + keepAll := func(string, os.FileInfo) bool { return true } + if err := copyDirContentsFiltered(srcDir, dstDir, keepAll); err != nil { + t.Fatalf("copyDirContentsFiltered should tolerate a vanished entry: %v", err) + } + if _, err := os.Stat(filepath.Join(dstDir, "a.txt")); err != nil { + t.Fatalf("a.txt should have been copied despite the vanished sibling: %v", err) + } +} diff --git a/audit/internal/app/support_bundle.go b/audit/internal/app/support_bundle.go index 6fa49d4..ddecbfc 100644 --- a/audit/internal/app/support_bundle.go +++ b/audit/internal/app/support_bundle.go @@ -752,6 +752,17 @@ func copyDirContentsFiltered(srcDir, dstDir string, keep func(rel string, info o func copyPath(src, dst string) error { info, err := os.Stat(src) if err != nil { + // src is read from a live export dir that bee's own SAT/task runner + // keeps writing to and renaming (e.g. a task dir's "_pending" -> + // "_done" transition) concurrently with this copy — a path present + // in the parent's os.ReadDir listing a moment ago disappearing by + // the time we get here is an expected race, not a real failure. + // Skip it; the next cycle will pick up wherever it landed under its + // new name. Aborting the whole copy over one renamed-away entry is + // what previously left blackbox stuck "degraded" indefinitely. + if os.IsNotExist(err) { + return nil + } return err } if info.IsDir() { @@ -760,6 +771,9 @@ func copyPath(src, dst string) error { } entries, err := os.ReadDir(src) if err != nil { + if os.IsNotExist(err) { + return nil + } return err } for _, entry := range entries { @@ -783,6 +797,9 @@ func copyPath(src, dst string) error { } in, err := os.Open(src) if err != nil { + if os.IsNotExist(err) { + return nil + } return err } defer in.Close() @@ -800,6 +817,12 @@ func copyPath(src, dst string) error { func copyPathFiltered(rootSrc, src, dst string, keep func(rel string, info os.FileInfo) bool) error { info, err := os.Stat(src) if err != nil { + // See the matching comment in copyPath: src disappearing between the + // parent's os.ReadDir and this Stat is an expected race against + // bee's own live task-dir renames, not a real error. + if os.IsNotExist(err) { + return nil + } return err } rel, err := filepath.Rel(rootSrc, src) @@ -815,6 +838,9 @@ func copyPathFiltered(rootSrc, src, dst string, keep func(rel string, info os.Fi } entries, err := os.ReadDir(src) if err != nil { + if os.IsNotExist(err) { + return nil + } return err } for _, entry := range entries {