Detect broken symlinks in the cache and fix them (#1994)

Currently, when an external process deletes a file that a symlink is
pointing to in the cache, but not the symlink itself, we will never fix
that and the cache will be permanently broken.

This makes the `AdvertiseCachedFile` sensitive to the possibility of
links being broken. Broken links are removed and the advertisement
process is retried to establish a functioning cache state again.

Author: markusthoemmes

pkg/paths/paths.go

@@ -47,21 +47,29 @@ func AdvertiseCachedFile(src, dst string) error {
 	if err != nil {
 		rel = src
 	}
-	// Check if the destination already exists.
-	if _, err := os.Stat(dst); err == nil {
-		// Since `src` is unadvertised, it is safe to remove it. Ideally we want this to succeeds,
-		// but we don't want to fail a build just because we couldn't clean up. This will be
-		// left for background clean up process based on age.
-		_ = os.Remove(src)
-		return nil
+
+	// Check what exists at dst using Lstat (doesn't follow symlinks).
+	// This lets us distinguish between "nothing exists" and "broken symlink".
+	if _, err := os.Lstat(dst); err == nil {
+		// Something exists at dst. Check if it's a valid symlink by following it.
+		if _, err := os.Stat(dst); err == nil {
+			// Valid symlink exists - another process already advertised.
+			// Clean up src since it's unadvertised and return.
+			_ = os.Remove(src)
+			return nil
+		}
+		// Broken symlink (Lstat succeeded but Stat failed) - remove it.
+		if err := os.Remove(dst); err != nil {
+			return fmt.Errorf("removing broken symlink %s: %w", dst, err)
+		}
 	}
+
 	// Create the symlink.
 	if err := os.Symlink(rel, dst); err != nil {
-		// Ignore already exists errors. We don't even want to do clean up here even when
-		// the symlink is pointing somewhere else, to avoid relying too much on file system
-		// remantics/eventual consistency, etc.
 		if errors.Is(err, os.ErrExist) {
-			return nil
+			// Race condition: something appeared between our Lstat check and Symlink.
+			// Re-run to handle it properly.
+			return AdvertiseCachedFile(src, dst)
 		}
 		return fmt.Errorf("linking (cached) %s to %s: %w", rel, dst, err)
 	}

pkg/paths/paths_test.go

@@ -23,12 +23,13 @@ func TestAdvertiseCachedFile(t *testing.T) {
 	tmpDir := t.TempDir()
 	src1 := tmpDir + "/src1.tmp"
 	src2 := tmpDir + "/src2.tmp"
+	src3 := tmpDir + "/src3.tmp"
 	content := "content"
-	if err := os.WriteFile(src1, []byte(content), 0644); err != nil {
-		t.Fatal(err)
-	}
-	if err := os.WriteFile(src2, []byte(content), 0644); err != nil {
-		t.Fatal(err)
+
+	for _, src := range []string{src1, src2, src3} {
+		if err := os.WriteFile(src, []byte(content), 0644); err != nil {
+			t.Fatal(err)
+		}
 	}
 	dst := tmpDir + "/target"
 	t.Run("dst does not exists", func(t *testing.T) {
@@ -64,4 +65,37 @@ func TestAdvertiseCachedFile(t *testing.T) {
 			t.Fatalf("src2 should be removed: %v", err)
 		}
 	})
+
+	t.Run("dst exists, but is broken", func(t *testing.T) {
+		// check the symlink
+		rel1, err := filepath.Rel(filepath.Dir(dst), src1)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if l, err := os.Readlink(dst); err != nil {
+			t.Fatal(err)
+		} else if l != rel1 {
+			t.Fatalf("unexpected symlink: %s != %s", l, src2)
+		}
+
+		// remove the target to break the symlink
+		if err := os.Remove(src1); err != nil {
+			t.Fatal(err)
+		}
+
+		// now advertise src3 to dst
+		if err := AdvertiseCachedFile(src3, dst); err != nil {
+			t.Fatal(err)
+		}
+		// check the symlink
+		rel2, err := filepath.Rel(filepath.Dir(dst), src3)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if l, err := os.Readlink(dst); err != nil {
+			t.Fatal(err)
+		} else if l != rel2 {
+			t.Fatalf("symlink should be updated: %s != %s", l, src2)
+		}
+	})
 }