loggerd: enhance deleter robustness for non-openpilot files

system/loggerd/deleter.py

@@ -45,28 +45,93 @@ def get_preserved_segments(dirs_by_creation: list[str]) -> set[str]:
   return preserved
 
 
+def safe_delete(path: str) -> bool:
+  """Safely delete a file or directory, handling various edge cases.
+
+  Returns True if deletion was successful, False otherwise.
+  """
+  try:
+    if os.path.islink(path):
+      # Handle symlinks - remove the link, not the target
+      os.unlink(path)
+    elif os.path.isfile(path):
+      # Handle regular files
+      os.remove(path)
+    elif os.path.isdir(path):
+      # Handle directories
+      shutil.rmtree(path)
+    else:
+      # Handle other types (sockets, FIFOs, etc.)
+      os.remove(path)
+    cloudlog.info(f"deleted {path}")
+    return True
+  except (OSError, PermissionError) as e:
+    cloudlog.exception(f"failed to delete {path}: {e}")
+    return False
+
+
 def deleter_thread(exit_event: threading.Event):
   while not exit_event.is_set():
     out_of_bytes = get_available_bytes(default=MIN_BYTES + 1) < MIN_BYTES
     out_of_percent = get_available_percent(default=MIN_PERCENT + 1) < MIN_PERCENT
 
     if out_of_percent or out_of_bytes:
-      dirs = listdir_by_creation(Paths.log_root())
+      log_root = Paths.log_root()
+
+      # First, check for and delete any non-directory items (stray files, symlinks, etc.)
+      try:
+        all_items = os.listdir(log_root)
+      except OSError:
+        cloudlog.exception(f"failed to list {log_root}")
+        exit_event.wait(.1)
+        continue
+
+      # Find non-directory items
+      non_dirs = []
+      for item in all_items:
+        item_path = os.path.join(log_root, item)
+        try:
+          # Check for symlinks first (before isdir, which follows symlinks)
+          if os.path.islink(item_path):
+            non_dirs.append(item)
+          elif not os.path.isdir(item_path):
+            non_dirs.append(item)
+        except OSError:
+          # If we can't stat it, treat it as a non-directory to attempt deletion
+          non_dirs.append(item)
+
+      # Delete non-directory items first (stray files, symlinks, etc.)
+      deleted_non_dir = False
+      for item in non_dirs:
+        item_path = os.path.join(log_root, item)
+        if safe_delete(item_path):
+          deleted_non_dir = True
+          break
+
+      # If we deleted a non-directory item, wait and continue to next iteration
+      if deleted_non_dir:
+        exit_event.wait(.1)
+        continue
+
+      # Get directories sorted by creation time (using the original function)
+      dirs = listdir_by_creation(log_root)
       preserved_dirs = get_preserved_segments(dirs)
 
-      # remove the earliest directory we can
+      # Remove the earliest directory we can
       for delete_dir in sorted(dirs, key=lambda d: (d in DELETE_LAST, d in preserved_dirs)):
-        delete_path = os.path.join(Paths.log_root(), delete_dir)
-
-        if any(name.endswith(".lock") for name in os.listdir(delete_path)):
-          continue
+        delete_path = os.path.join(log_root, delete_dir)
 
         try:
-          cloudlog.info(f"deleting {delete_path}")
-          shutil.rmtree(delete_path)
-          break
+          # Check for lock files
+          if any(name.endswith(".lock") for name in os.listdir(delete_path)):
+            continue
         except OSError:
-          cloudlog.exception(f"issue deleting {delete_path}")
+          # If we can't list the directory, try to delete it anyway
+          pass
+
+        if safe_delete(delete_path):
+          break
+
       exit_event.wait(.1)
     else:
       exit_event.wait(30)

system/loggerd/tests/test_deleter.py

@@ -1,3 +1,4 @@
+import os
 import time
 import threading
 from collections import namedtuple
@@ -7,6 +8,7 @@
 import openpilot.system.loggerd.deleter as deleter
 from openpilot.common.timeout import Timeout, TimeoutException
 from openpilot.system.loggerd.tests.loggerd_tests_common import UploaderTestCase
+from openpilot.system.hardware.hw import Paths
 
 Stats = namedtuple("Stats", ['f_bavail', 'f_blocks', 'f_frsize'])
 
@@ -115,3 +117,199 @@ def test_no_delete_with_lock_file(self):
     self.join_thread()
 
     assert f_path.exists(), "File deleted when locked"
+
+  def test_delete_stray_file_in_log_root(self):
+    """Test deletion of a stray file directly in log root."""
+    log_root = Path(Paths.log_root())
+    stray_file = log_root / "stray_file.txt"
+    stray_file.write_text("stray content")
+
+    self.start_thread()
+    try:
+      with Timeout(2, "Timeout waiting for stray file to be deleted"):
+        while stray_file.exists():
+          time.sleep(0.01)
+    finally:
+      self.join_thread()
+
+    assert not stray_file.exists(), "Stray file not deleted"
+
+  def test_delete_symlink_in_log_root(self):
+    """Test deletion of a symlink directly in log root."""
+    import tempfile
+    log_root = Path(Paths.log_root())
+
+    # Create a target file outside log root
+    with tempfile.TemporaryDirectory() as tmpdir:
+      target_file = Path(tmpdir) / "target.txt"
+      target_file.write_text("target content")
+
+      # Create a symlink in log root
+      symlink = log_root / "symlink_to_target"
+      symlink.symlink_to(target_file)
+
+      self.start_thread()
+      try:
+        with Timeout(2, "Timeout waiting for symlink to be deleted"):
+          while symlink.is_symlink():
+            time.sleep(0.01)
+      finally:
+        self.join_thread()
+
+      assert not symlink.is_symlink(), "Symlink not deleted"
+      assert target_file.exists(), "Target file was deleted (should only delete symlink)"
+
+  def test_delete_nested_directory_structure(self):
+    """Test deletion of deeply nested directory structures."""
+    log_root = Path(Paths.log_root())
+    nested_dir = log_root / "nested" / "deep" / "structure"
+    nested_dir.mkdir(parents=True, exist_ok=True)
+
+    # Create files at various levels
+    (log_root / "nested" / "file1.txt").write_text("content1")
+    (log_root / "nested" / "deep" / "file2.txt").write_text("content2")
+    (nested_dir / "file3.txt").write_text("content3")
+
+    self.start_thread()
+    try:
+      with Timeout(2, "Timeout waiting for nested directory to be deleted"):
+        while (log_root / "nested").exists():
+          time.sleep(0.01)
+    finally:
+      self.join_thread()
+
+    assert not (log_root / "nested").exists(), "Nested directory not deleted"
+
+  def test_delete_mixed_items_in_log_root(self):
+    """Test deletion of various types of items: files, symlinks, directories."""
+    import tempfile
+    log_root = Path(Paths.log_root())
+
+    # Create various items
+    items = []
+
+    # Regular file
+    regular_file = log_root / "regular.txt"
+    regular_file.write_text("regular content")
+    items.append(regular_file)
+
+    # Empty directory
+    empty_dir = log_root / "empty_dir"
+    empty_dir.mkdir(exist_ok=True)
+    items.append(empty_dir)
+
+    # Directory with files
+    dir_with_files = log_root / "dir_with_files"
+    dir_with_files.mkdir(exist_ok=True)
+    (dir_with_files / "file.txt").write_text("content")
+    items.append(dir_with_files)
+
+    # Symlink to file outside log root
+    with tempfile.TemporaryDirectory() as tmpdir:
+      target = Path(tmpdir) / "target.txt"
+      target.write_text("target")
+      symlink = log_root / "link_to_file"
+      symlink.symlink_to(target)
+      items.append(symlink)
+
+      self.start_thread()
+      try:
+        with Timeout(5, "Timeout waiting for mixed items to be deleted"):
+          while any(item.exists() for item in items if not item.is_symlink()) or any(item.is_symlink() for item in items):
+            time.sleep(0.01)
+      finally:
+        self.join_thread()
+
+      # All items should be deleted
+      for item in items:
+        if item.is_symlink():
+          assert not item.is_symlink(), f"Symlink {item} not deleted"
+        else:
+          assert not item.exists(), f"{item} not deleted"
+
+      # Target should still exist
+      assert target.exists(), "Target file was deleted (should only delete symlink)"
+
+  def test_delete_with_permission_errors(self):
+    """Test that deleter continues after encountering permission errors."""
+    # Create two directories
+    dir1_path = self.make_file_with_data(self.seg_format.format(0), self.f_type)
+    dir2_path = self.make_file_with_data(self.seg_format.format(1), self.f_type)
+
+    # Make dir1 read-only (simulate permission error)
+    # Note: On Linux, making a directory read-only doesn't prevent deletion of the directory itself,
+    # only prevents writing to its contents. The deleter may still delete it.
+    dir1 = dir1_path.parent
+    try:
+      os.chmod(dir1, 0o444)
+
+      self.start_thread()
+      try:
+        # dir2 should still be deleted even if dir1 fails
+        with Timeout(3, "Timeout waiting for dir2 to be deleted"):
+          while dir2_path.exists():
+            time.sleep(0.01)
+      finally:
+        self.join_thread()
+
+      # Restore permissions for cleanup (if directory still exists)
+      if dir1.exists():
+        os.chmod(dir1, 0o755)
+
+    except (OSError, PermissionError):
+      # On some systems we can't change permissions, skip this test
+      if dir1.exists():
+        os.chmod(dir1, 0o755)
+      return
+
+  def test_delete_special_characters_in_names(self):
+    """Test deletion of files/directories with special characters in names."""
+    log_root = Path(Paths.log_root())
+
+    # Create items with special characters
+    special_names = [
+      "file with spaces.txt",
+      "file-with-dashes.txt",
+      "file_with_underscores.txt",
+      "file.multiple.dots.txt",
+    ]
+
+    items = []
+    for name in special_names:
+      item = log_root / name
+      item.write_text("content")
+      items.append(item)
+
+    self.start_thread()
+    try:
+      with Timeout(3, "Timeout waiting for special character files to be deleted"):
+        while any(item.exists() for item in items):
+          time.sleep(0.01)
+    finally:
+      self.join_thread()
+
+    for item in items:
+      assert not item.exists(), f"{item} not deleted"
+
+  def test_delete_empty_files_and_directories(self):
+    """Test deletion of empty files and directories."""
+    log_root = Path(Paths.log_root())
+
+    # Create empty file
+    empty_file = log_root / "empty.txt"
+    empty_file.touch()
+
+    # Create empty directory
+    empty_dir = log_root / "empty_dir"
+    empty_dir.mkdir(exist_ok=True)
+
+    self.start_thread()
+    try:
+      with Timeout(2, "Timeout waiting for empty items to be deleted"):
+        while empty_file.exists() or empty_dir.exists():
+          time.sleep(0.01)
+    finally:
+      self.join_thread()
+
+    assert not empty_file.exists(), "Empty file not deleted"
+    assert not empty_dir.exists(), "Empty directory not deleted"