Fix deleter crash

system/loggerd/deleter.py

@@ -51,14 +51,38 @@ def deleter_thread(exit_event: threading.Event):
     out_of_percent = get_available_percent(default=MIN_PERCENT + 1) < MIN_PERCENT
 
     if out_of_percent or out_of_bytes:
+      # delete stray files or broken symlinks first
+      # fixes issue where non-directories take up space but get ignored
+      all_items = os.listdir(Paths.log_root())
+      file_deleted = False
+
+      for item in all_items:
+        item_path = os.path.join(Paths.log_root(), item)
+        if not os.path.isdir(item_path) and not item.endswith(".lock"):
+          try:
+            cloudlog.info(f"deleting stray item {item_path}")
+            os.remove(item_path)
+            file_deleted = True
+            break    # re-evaluate space usage upon file deletion
+          except OSError:
+            cloudlog.exception(f"issue deleting stray {item_path}")
+
+      if file_deleted:
+        exit_event.wait(.1)
+        continue
+
       dirs = listdir_by_creation(Paths.log_root())
       preserved_dirs = get_preserved_segments(dirs)
 
       # 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)):
+        try:
+          if any(name.endswith(".lock") for name in os.listdir(delete_path)):
+            continue
+        except OSError:
+          cloudlog.exception(f"issue listing {delete_path}")
           continue
 
         try:

system/loggerd/tests/test_deleter.py

@@ -1,12 +1,14 @@
 import time
 import threading
+import os
 from collections import namedtuple
 from pathlib import Path
 from collections.abc import Sequence
 
 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'])
 
@@ -65,30 +67,34 @@ def assertDeleteOrder(self, f_paths: Sequence[Path], timeout: int = 5) -> None:
     assert deleted_order == f_paths, "Files not deleted in expected order"
 
   def test_delete_order(self):
-    self.assertDeleteOrder([
-      self.make_file_with_data(self.seg_format.format(0), self.f_type),
-      self.make_file_with_data(self.seg_format.format(1), self.f_type),
-      self.make_file_with_data(self.seg_format2.format(0), self.f_type),
-    ])
+    self.assertDeleteOrder(
+      [
+        self.make_file_with_data(self.seg_format.format(0), self.f_type),
+        self.make_file_with_data(self.seg_format.format(1), self.f_type),
+        self.make_file_with_data(self.seg_format2.format(0), self.f_type),
+      ]
+    )
 
   def test_delete_many_preserved(self):
-    self.assertDeleteOrder([
-      self.make_file_with_data(self.seg_format.format(0), self.f_type),
-      self.make_file_with_data(self.seg_format.format(1), self.f_type, preserve_xattr=deleter.PRESERVE_ATTR_VALUE),
-      self.make_file_with_data(self.seg_format.format(2), self.f_type),
-    ] + [
-      self.make_file_with_data(self.seg_format2.format(i), self.f_type, preserve_xattr=deleter.PRESERVE_ATTR_VALUE)
-      for i in range(5)
-    ])
+    self.assertDeleteOrder(
+      [
+        self.make_file_with_data(self.seg_format.format(0), self.f_type),
+        self.make_file_with_data(self.seg_format.format(1), self.f_type, preserve_xattr=deleter.PRESERVE_ATTR_VALUE),
+        self.make_file_with_data(self.seg_format.format(2), self.f_type),
+      ]
+      + [self.make_file_with_data(self.seg_format2.format(i), self.f_type, preserve_xattr=deleter.PRESERVE_ATTR_VALUE) for i in range(5)]
+    )
 
   def test_delete_last(self):
-    self.assertDeleteOrder([
-      self.make_file_with_data(self.seg_format.format(1), self.f_type),
-      self.make_file_with_data(self.seg_format2.format(0), self.f_type),
-      self.make_file_with_data(self.seg_format.format(0), self.f_type, preserve_xattr=deleter.PRESERVE_ATTR_VALUE),
-      self.make_file_with_data("boot", self.seg_format[:-4]),
-      self.make_file_with_data("crash", self.seg_format2[:-4]),
-    ])
+    self.assertDeleteOrder(
+      [
+        self.make_file_with_data(self.seg_format.format(1), self.f_type),
+        self.make_file_with_data(self.seg_format2.format(0), self.f_type),
+        self.make_file_with_data(self.seg_format.format(0), self.f_type, preserve_xattr=deleter.PRESERVE_ATTR_VALUE),
+        self.make_file_with_data("boot", self.seg_format[:-4]),
+        self.make_file_with_data("crash", self.seg_format2[:-4]),
+      ]
+    )
 
   def test_no_delete_when_available_space(self):
     f_path = self.make_file_with_data(self.seg_dir, self.f_type)
@@ -115,3 +121,58 @@ def test_no_delete_with_lock_file(self):
     self.join_thread()
 
     assert f_path.exists(), "File deleted when locked"
+
+  def test_delete_mixed_contents(self):
+    fake_seg_path = os.path.join(Paths.log_root(), "2024-05-20--12-00-00--0")
+    with open(fake_seg_path, 'w') as f:
+      f.write("file masquerading as a directory")
+
+    stray_log_path = os.path.join(Paths.log_root(), "random.log")
+    with open(stray_log_path, 'w') as f:
+      f.write("data")
+
+    broken_link_path = os.path.join(Paths.log_root(), "broken_link")
+    try:
+      os.symlink("does_not_exist", broken_link_path)
+    except OSError:
+      pass
+
+    real_dir = os.path.join(Paths.log_root(), "real_dir")
+    os.mkdir(real_dir)
+
+    nested = os.path.join(real_dir, "a", "b", "c")
+    os.makedirs(nested, exist_ok=True)
+    for i in range(2):
+      with open(os.path.join(real_dir, f"root_{i}.txt"), "w") as f:
+        f.write("x")
+      with open(os.path.join(nested, f"leaf_{i}.txt"), "w") as f:
+        f.write("y")
+
+    valid_link_path = os.path.join(Paths.log_root(), "valid_link")
+    try:
+      os.symlink(real_dir, valid_link_path)
+    except OSError:
+      pass
+
+    valid_seg_path = self.make_file_with_data(self.seg_format.format(1), self.f_type, 1)
+
+    self.start_thread()
+    try:
+      with Timeout(5, "Timeout waiting for mixed deletions"):
+        while (
+          os.path.exists(fake_seg_path)
+          or os.path.exists(stray_log_path)
+          or os.path.exists(broken_link_path)
+          or valid_seg_path.exists()
+        ):
+          time.sleep(0.01)
+    finally:
+      self.join_thread()
+
+    assert not os.path.exists(fake_seg_path)
+    assert not os.path.exists(stray_log_path)
+    assert not os.path.exists(broken_link_path)
+    assert not valid_seg_path.exists()
+
+    # valid alias should remain as the target is a real directory tree
+    assert os.path.exists(valid_link_path)