Refine cgroup deletion behavior and logging

pyisolate/cgroup.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import ctypes
+import errno
 import logging
 import os
 import threading
@@ -66,15 +67,37 @@ def attach_current(path: Path | None) -> None:
 
 
 def delete(path: Path | None) -> None:
-    """Remove an empty cgroup."""
+    """Remove a cgroup directory with best-effort thread drain."""
     if path is None:
         return
+
+    parent_threads = path.parent / "cgroup.threads"
+    child_threads = path / "cgroup.threads"
+
+    # Best-effort drain of lingering tasks so rmdir has a chance to succeed.
+    try:
+        tids = child_threads.read_text().splitlines()
+    except (OSError, PermissionError, FileNotFoundError):
+        tids = []
+
+    for tid in tids:
+        try:
+            parent_threads.write_text(tid)
+        except (OSError, PermissionError, FileNotFoundError):
+            # Still attempt rmdir and rely on errno-aware logging below.
+            break
+
     try:
-        for f in path.iterdir():
-            f.unlink(missing_ok=True)
         path.rmdir()
-    except (OSError, PermissionError, FileNotFoundError) as exc:
-        log.warning("Failed to delete cgroup %s: %s", path, exc)
+    except FileNotFoundError as exc:
+        log.warning("Cgroup path missing while deleting %s: %s", path, exc)
+    except PermissionError as exc:
+        log.warning("Permission denied deleting cgroup %s: %s", path, exc)
+    except OSError as exc:
+        if exc.errno in {errno.EBUSY, errno.ENOTEMPTY}:
+            log.warning("Cgroup %s is busy/non-empty; skipping delete: %s", path, exc)
+        else:
+            log.warning("Failed to delete cgroup %s: %s", path, exc)
 
 
 def list_children() -> list[Path]:

tests/test_cgroup.py

@@ -1,4 +1,5 @@
 import logging
+import errno
 import sys
 from pathlib import Path
 
@@ -39,17 +40,43 @@ def failing_write(self, data):
     assert "Failed to attach thread" in caplog.text
 
 
-def test_delete_logs_warning_on_error(tmp_path, monkeypatch, caplog):
+def test_delete_does_not_unlink_files(tmp_path, monkeypatch):
+    path = tmp_path / "cg"
+    path.mkdir()
+    (path / "some.file").write_text("x")
+
+    def unlink_should_not_be_called(self, *args, **kwargs):  # pragma: no cover
+        raise AssertionError("unlink() should not be used by delete()")
+
+    monkeypatch.setattr(Path, "unlink", unlink_should_not_be_called)
+    cgroup.delete(path)
+    assert path.exists()
+
+
+def test_delete_logs_busy_warning(tmp_path, monkeypatch, caplog):
     path = tmp_path / "cg"
     path.mkdir()
 
     def failing_rmdir(self):
-        raise OSError("boom")
+        raise OSError(errno.ENOTEMPTY, "Directory not empty")
+
+    monkeypatch.setattr(Path, "rmdir", failing_rmdir)
+    with caplog.at_level(logging.WARNING, logger=cgroup.__name__):
+        cgroup.delete(path)
+    assert "busy/non-empty" in caplog.text
+
+
+def test_delete_logs_permission_error(tmp_path, monkeypatch, caplog):
+    path = tmp_path / "cg"
+    path.mkdir()
+
+    def failing_rmdir(self):
+        raise PermissionError("denied")
 
     monkeypatch.setattr(Path, "rmdir", failing_rmdir)
     with caplog.at_level(logging.WARNING, logger=cgroup.__name__):
         cgroup.delete(path)
-    assert "Failed to delete cgroup" in caplog.text
+    assert "Permission denied deleting cgroup" in caplog.text