Fix uninstall data-delete crash and uv cache-lock hang

Found while exercising `spin uninstall` end-to-end against a real default
data dir on this machine.

1. _delete_owned crashed mid-rmtree with FileNotFoundError on studio.sqlite3-
   shm/-wal: SQLite checkpoints remove WAL sidecar files between rmtree's
   directory listing and its per-file unlink, aborting the delete and leaving
   the data dir half-deleted. Now pass an onerror that tolerates
   FileNotFoundError (already-gone files) but still surfaces real errors like
   PermissionError, plus a best-effort final sweep.

2. _uv_cleanup hung indefinitely when the uv cache lock was held. The first
   attempt used subprocess.run(timeout=), which is unreliable here: `uv cache
   clean` spawns children that keep its pipes open, so run()'s internal
   communicate() blocked ~94 minutes past a 120 s bound. In particular,
   `spin uninstall` invoked via `uvx --from ...` runs inside a uv session whose
   ancestor holds the cache lock, so the child deadlocks on its own parent.
   Rewrote each step to Popen in its own session with a hard wall-clock
   deadline that kills the whole process group; on timeout it reports
   "Timed out ... left as-is" instead of hanging.

Verified: real `spin uninstall` now removes the data dir cleanly (no crash,
no debris) and completes in bounded time (~241 s worst case, exit 0) with the
uv-cleanup steps degrading gracefully. Unit tests added for the WAL race,
permission-error propagation, and the wall-clock bound (with a fake hanging
uv); no orphaned children leak from the group kill.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: SushantGautam

simpleaudit_studio/lifecycle.py

@@ -15,6 +15,7 @@
 import socket
 import subprocess
 import sys
+import time
 import uuid
 from pathlib import Path
 
@@ -143,7 +144,22 @@ def _delete_owned(instance: Instance) -> None:
     if not safe or data.is_symlink():
         raise SystemExit(f"Refusing to delete unexpected data path: {data}")
     if data.exists():
-        shutil.rmtree(data)
+        # SQLite checkpoints can remove -wal/-shm between rmtree's listing and
+        # its unlink; tolerate files that are already gone but surface real
+        # errors (e.g. permission denied) instead of aborting mid-delete.
+        def _rm_tolerate(fn, path, exc_info):
+            if not isinstance(exc_info[1], FileNotFoundError):
+                raise exc_info[1]
+
+        shutil.rmtree(data, onerror=_rm_tolerate)
+        # Best-effort sweep in case a hot journal reappeared as the last step.
+        for leftover in data.iterdir() if data.exists() else []:
+            try:
+                shutil.rmtree(leftover) if leftover.is_dir() else leftover.unlink()
+            except FileNotFoundError:
+                pass
+        if data.exists() and not any(data.iterdir()):
+            data.rmdir()
     if instance.mode == "dev":
         for suffix in ("", "-wal", "-shm", "-journal"):
             target = Path(str(instance.db) + suffix)
@@ -153,6 +169,34 @@ def _delete_owned(instance: Instance) -> None:
     print("Removed instance-owned data. Repository and external results were preserved.")
 
 
+class _Completed:
+    """Minimal stand-in for a finished subprocess result (only returncode used)."""
+
+    def __init__(self, returncode: int):
+        self.returncode = returncode
+
+
+def _kill_group(proc: subprocess.Popen) -> None:
+    """Terminate then kill the whole session/group a child started in."""
+    try:
+        os.killpg(os.getpgid(proc.pid), signal.SIGTERM)
+    except (ProcessLookupError, PermissionError):
+        pass
+    try:
+        proc.wait(timeout=5)
+        return
+    except subprocess.TimeoutExpired:
+        pass
+    try:
+        os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
+    except (ProcessLookupError, PermissionError):
+        pass
+    try:
+        proc.wait(timeout=2)
+    except subprocess.TimeoutExpired:
+        pass
+
+
 def _uv_cleanup(mode: str) -> None:
     uv = shutil.which("uv")
     if not uv:
@@ -161,13 +205,40 @@ def _uv_cleanup(mode: str) -> None:
     # Package-scoped cache cleanup, NEVER `uv cache clean` without a name.
     # A uvx ephemeral environment may remain: uv offers no reliable per-tool
     # selective deletion of all shared cached environments.
+    #
+    # Each step is hard-bounded by wall clock. `subprocess.run(timeout=)` is NOT
+    # sufficient: `uv cache clean` spawns children that keep its pipes open, so
+    # run()'s internal communicate() can block far past the deadline (observed
+    # ~94 min vs a 120 s bound). We therefore run each step in its own session
+    # and kill the whole group on a real deadline.
+    def _bounded(cmd, timeout):
+        proc = subprocess.Popen(
+            cmd,
+            stdout=subprocess.DEVNULL,
+            stderr=subprocess.STDOUT,
+            start_new_session=True,
+        )
+        deadline = time.monotonic() + timeout
+        while True:
+            rc = proc.poll()
+            if rc is not None:
+                return _Completed(rc)
+            if time.monotonic() >= deadline:
+                _kill_group(proc)
+                return None
+            time.sleep(0.5)
+
     for name in ("simpleaudit-studio", "open-webui"):
-        result = subprocess.run([uv, "cache", "clean", name], check=False)
-        if result.returncode:
+        result = _bounded([uv, "cache", "clean", name], timeout=120)
+        if result is None:
+            print(f"Timed out clearing uv cache for {name}; left as-is.")
+        elif result.returncode:
             print(f"Could not clear uv cache for {name} (exit {result.returncode}).")
     if mode == "spin":
-        result = subprocess.run([uv, "tool", "uninstall", "simpleaudit-studio"], check=False, capture_output=True, text=True)
-        if result.returncode == 0:
+        result = _bounded([uv, "tool", "uninstall", "simpleaudit-studio"], timeout=60)
+        if result is None:
+            print("Timed out removing uv tool installation; left as-is.")
+        elif result.returncode == 0:
             print("Removed permanent uv tool installation.")
         else:
             print("No permanent uv tool installation removed (uvx does not install one).")

tests/test_lifecycle.py

@@ -70,6 +70,59 @@ def test_delete_owned_data_only(environment):
     assert (checkout / "manage.py").exists()
 
 
+def test_delete_owned_tolerates_wal_file_vanishing_during_rmtree(environment, monkeypatch):
+    """A SQLite checkpoint can remove -wal/-shm after rmtree lists them.
+
+    Simulate that by making unlink raise FileNotFoundError for one file; the
+    delete must still complete rather than abort mid-way and leave debris.
+    """
+    import shutil as _shutil
+
+    checkout = environment / "repo"
+    checkout.mkdir()
+    instance = lifecycle.Instance("dev", checkout)
+    data = instance.data
+    data.mkdir(parents=True)
+    (data / "studio.sqlite3").write_text("db")
+    (data / "studio.sqlite3-wal").write_text("wal")
+    (data / "studio.sqlite3-shm").write_text("shm")
+    (data / "nested").mkdir()
+    (data / "nested" / "x.txt").write_text("x")
+
+    real_rmtree = _shutil.rmtree
+    flaky = data / "studio.sqlite3-shm"
+
+    def racing_rmtree(path, *args, **kwargs):
+        # Emulate the checkpoint: drop the shm file right before rmtree walks it.
+        if Path(path) == data:
+            try:
+                flaky.unlink()
+            except FileNotFoundError:
+                pass
+        return real_rmtree(path, *args, **kwargs)
+
+    monkeypatch.setattr(lifecycle.shutil, "rmtree", racing_rmtree)
+    lifecycle._delete_owned(instance)
+    assert not data.exists()
+
+
+def test_delete_owned_surfaces_real_errors_not_just_missing_files(environment, monkeypatch):
+    """Permission errors must propagate; only FileNotFoundError is tolerated."""
+    checkout = environment / "repo"
+    checkout.mkdir()
+    instance = lifecycle.Instance("dev", checkout)
+    data = instance.data
+    data.mkdir(parents=True)
+    (data / "locked").write_text("x")
+
+    def boom(*a, **k):
+        raise PermissionError(13, "denied", str(data))
+
+    monkeypatch.setattr(lifecycle.shutil, "rmtree", boom)
+    with pytest.raises(PermissionError):
+        lifecycle._delete_owned(instance)
+
+
 def test_default_port_selection_has_no_duplicate_ports():
     assert len(set(lifecycle._requested_ports(8000).values())) == 4
     assert lifecycle._requested_ports(8100)["public"] == 8100
@@ -91,13 +144,71 @@ def test_live_rejects_unowned_pid():
 def test_uninstall_only_cleans_named_uv_packages(monkeypatch):
     calls = []
     monkeypatch.setattr(lifecycle.shutil, "which", lambda _: "/usr/bin/uv")
-    class Result:
-        returncode = 0
-    monkeypatch.setattr(lifecycle.subprocess, "run", lambda args, **kwargs: (calls.append(args), Result())[1])
+
+    class DoneProc:
+        pid = 42
+
+        def poll(self):
+            return 0  # completes immediately
+
+        def wait(self, timeout=None):
+            return None
+
+    monkeypatch.setattr(
+        lifecycle.subprocess,
+        "Popen",
+        lambda cmd, **kw: (calls.append(cmd), DoneProc())[1],
+    )
     lifecycle._uv_cleanup("dev")
     assert calls == [["/usr/bin/uv", "cache", "clean", "simpleaudit-studio"],
                      ["/usr/bin/uv", "cache", "clean", "open-webui"]]
-    assert not any(args == ["/usr/bin/uv", "cache", "clean"] for args in calls)
+    assert not any(c == ["/usr/bin/uv", "cache", "clean"] for c in calls)
+
+
+def test_uv_cleanup_is_bounded_by_wall_clock_not_run_timeout(monkeypatch, capsys):
+    """`uv cache clean` can hold pipes open so subprocess.run(timeout=) blocks far
+    past its deadline (observed ~94 min vs 120 s). Cleanup must instead enforce a
+    real wall-clock bound: return quickly and report a timeout when a step hangs."""
+    import subprocess as _sp
+
+    monkeypatch.setattr(lifecycle.shutil, "which", lambda _: "/usr/bin/uv")
+
+    class HangingProc:
+        pid = 999999
+
+        def poll(self):
+            return None  # never finishes
+
+        def wait(self, timeout=None):
+            raise _sp.TimeoutExpired("cmd", timeout)
+
+    killed = []
+
+    def fake_popen(cmd, **kwargs):
+        assert kwargs.get("start_new_session") is True
+        return HangingProc()
+
+    def fake_kill_group(proc):
+        killed.append(proc)
+
+    monkeypatch.setattr(lifecycle.subprocess, "Popen", fake_popen)
+    monkeypatch.setattr(lifecycle, "_kill_group", fake_kill_group)
+
+    # Fast-forward the clock so each step's deadline elapses after one iteration,
+    # while still proving the loop terminates (vs the old 94-minute hang).
+    clock = {"now": 0.0}
+
+    def fake_monotonic():
+        clock["now"] += 1000.0  # jump past any deadline instantly
+        return clock["now"]
+
+    monkeypatch.setattr(lifecycle.time, "monotonic", fake_monotonic)
+    monkeypatch.setattr(lifecycle.time, "sleep", lambda *_: None)
+    lifecycle._uv_cleanup("spin")
+    assert killed, "hung steps must be killed via the group killer"
+    out = capsys.readouterr().out
+    assert "Timed out clearing uv cache" in out
+    assert "Timed out removing uv tool installation" in out
 
 
 def test_uninstall_cannot_use_live_external_db(environment, monkeypatch):