fix(test): own /dev/shm reset scratch by a flock, not a pid (#419)

A browser server inside an --ipc=host container shares the host's /dev/shm
but not its PID namespace, so the pid probe saw live servers' scratch folders
as orphans and deleted them. A live owner now holds a flock on owner.lock in
its folder; the kernel drops it when the owner dies, in any namespace. The
cleaner removes only held-named folders whose lock is free and leaves legacy
and staging folders alone. Without flock (Windows) the scratch stays on disk.

tests/browser/fast_reset.py

@@ -28,6 +28,11 @@ import weakref
 from pathlib import Path
 from typing import Any, Callable, TypeVar
 
+try:
+    import fcntl
+except ImportError:  # Windows: no flock, and no /dev/shm to share
+    fcntl = None
+
 T = TypeVar("T")
 
 # Set to 1 to run seeders on the live database as before #384 (the equivalence
@@ -43,8 +48,13 @@ MEMORY_ROOT = Path("/dev/shm")
 # the browsers and the other workers; with less free than this, the scratch
 # copy goes on disk instead.
 MEMORY_FREE_BYTES = 32 * 1024 * 1024
-# Scratch folders are named <prefix><owner pid>-<random>.
+# Scratch folders are created as <prefix>new-<owner pid>-<random>, locked, then
+# renamed to <prefix>held-<owner pid>-<random>. The pid is for people only:
+# ownership is the flock on OWNER_LOCK, which every PID namespace sees (#419).
 SCRATCH_PREFIX = "pm-reset-"
+STAGING_PREFIX = f"{SCRATCH_PREFIX}new-"
+HELD_PREFIX = f"{SCRATCH_PREFIX}held-"
+OWNER_LOCK = "owner.lock"
 
 
 def _quote(name: str) -> str:
@@ -96,29 +106,40 @@ def table_digests(conn: sqlite3.Connection, schema: str = "main") -> dict[str, s
     return result
 
 
-def _process_alive(pid: int) -> bool:
+def _hold(folder: Path, *, create: bool) -> int:
+    """Take ``folder``'s owner lock without waiting; the open descriptor."""
+    flags = os.O_RDWR | (os.O_CREAT if create else 0)
+    fd = os.open(folder / OWNER_LOCK, flags, 0o600)
     try:
-        os.kill(pid, 0)
-    except ProcessLookupError:
-        return False
+        fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
     except OSError:
-        return True  # it exists but belongs to someone else
-    return True
+        os.close(fd)
+        raise
+    return fd
 
 
 def remove_orphaned_scratch(root: Path) -> None:
     """Remove scratch folders of servers that died without cleaning up.
 
     A server killed outright (SIGKILL, a crash) never runs ``close``, and
-    tmpfs keeps its folder in memory until reboot. The folder name carries
-    the owner's process id; a folder whose process is gone is removed.
+    tmpfs keeps its folder in memory until reboot. A live owner holds the
+    flock on the folder's owner lock; the kernel drops it when the owner
+    dies, in whatever PID namespace it ran (a container with ``--ipc=host``
+    shares /dev/shm but not the pids, #419). A folder whose lock is free is
+    removed. Folders without a lock (staging, or named before #419) cannot
+    be proven orphaned and are left alone.
     """
-    if os.name != "posix":
-        return  # os.kill(pid, 0) would terminate the process on Windows
-    for folder in root.glob(f"{SCRATCH_PREFIX}*-*"):
-        pid = folder.name[len(SCRATCH_PREFIX) :].split("-", 1)[0]
-        if pid.isdigit() and not _process_alive(int(pid)):
+    if fcntl is None:
+        return
+    for folder in root.glob(f"{HELD_PREFIX}*"):
+        try:
+            fd = _hold(folder, create=False)
+        except OSError:
+            continue  # held by its owner, no lock to judge by, or gone
+        try:
             shutil.rmtree(folder, ignore_errors=True)
+        finally:
+            os.close(fd)
 
 
 class _SeedApi:
@@ -162,6 +183,7 @@ class ScratchSeeder:
         self._fallback = Path(directory)
         self._memory_root = memory_root
         self._directory: Path | None = None
+        self._owner_lock: int | None = None
         self._on_live = on_live
         self._scratch = None
         self._unsynced: "weakref.WeakSet[sqlite3.Connection]" = weakref.WeakSet()
@@ -266,6 +288,9 @@ class ScratchSeeder:
         if self._directory is not None:
             shutil.rmtree(self._directory, ignore_errors=True)
             self._directory = None
+        if self._owner_lock is not None:
+            os.close(self._owner_lock)  # after the removal: never a free lock
+            self._owner_lock = None
 
     def _scratch_directory(self) -> Path:
         """A folder in memory (tmpfs) where there is one, else ``directory``.
@@ -274,10 +299,35 @@ class ScratchSeeder:
         kernel is writing back the pages they rewrite.
         """
         root = self._memory_root
-        if root is not None and root.is_dir() and os.access(root, os.W_OK):
+        if (
+            fcntl is not None
+            and root is not None
+            and root.is_dir()
+            and os.access(root, os.W_OK)
+        ):
             remove_orphaned_scratch(root)
             if shutil.disk_usage(root).free >= MEMORY_FREE_BYTES:
-                prefix = f"{SCRATCH_PREFIX}{os.getpid()}-"
-                return Path(tempfile.mkdtemp(prefix=prefix, dir=root))
+                held = self._held_memory_directory(root)
+                if held is not None:
+                    return held
         self._fallback.mkdir(parents=True, exist_ok=True)
         return self._fallback
+
+    def _held_memory_directory(self, root: Path) -> Path | None:
+        """A new folder under ``root`` that this seeder holds the lock of.
+
+        It is locked under the staging name, which no cleaner judges, and
+        only then renamed: no cleaner ever sees it held-named and unlocked.
+        """
+        staging = Path(
+            tempfile.mkdtemp(prefix=f"{STAGING_PREFIX}{os.getpid()}-", dir=root)
+        )
+        try:
+            fd = _hold(staging, create=True)
+        except OSError:
+            shutil.rmtree(staging, ignore_errors=True)
+            return None  # no locks on this filesystem: use the disk
+        held = root / (HELD_PREFIX + staging.name[len(STAGING_PREFIX) :])
+        os.rename(staging, held)
+        self._owner_lock = fd
+        return held

tests/test_browser_fast_reset.py

@@ -22,6 +22,9 @@ from database.operations import PromptDatabase
 from tests.browser import fast_reset
 from tests.browser.fast_reset import ScratchSeeder, table_digests
 
+# The owner lock of /dev/shm scratch folders needs POSIX flock (#419).
+HAS_FLOCK = unittest.skipUnless(os.name == "posix", "needs POSIX flock")
+
 
 def digests(db: PromptDatabase) -> dict:
     return table_digests(db.model.get_connection())
@@ -275,6 +278,7 @@ class TestScratchSeeder(unittest.TestCase):
         self.assertEqual(digests(self.live), expected)
         self.assertFalse((self.root / "unused").exists())
 
+    @HAS_FLOCK
     def test_the_scratch_lives_under_the_memory_root_when_there_is_one(self):
         memory = self.root / "memory"
         memory.mkdir()
@@ -288,6 +292,16 @@ class TestScratchSeeder(unittest.TestCase):
         seeder.close()
         self.assertEqual(list(memory.iterdir()), [])
 
+    @unittest.skipIf(os.name == "posix", "without flock the memory root is unused")
+    def test_without_flock_the_scratch_stays_on_disk(self):
+        memory = self.root / "memory"
+        memory.mkdir()
+        seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory)
+        self.addCleanup(seeder.close)
+        seeder.run(reseed)
+        self.assertEqual(list(memory.iterdir()), [])
+        self.assertTrue((self.root / "fallback" / "seed.db").exists())
+
     def test_a_full_memory_root_puts_the_scratch_on_disk(self):
         memory = self.root / "memory"
         memory.mkdir()
@@ -303,26 +317,32 @@ class TestScratchSeeder(unittest.TestCase):
         self.assertEqual(list(memory.iterdir()), [])
         self.assertTrue((self.root / "fallback" / "seed.db").exists())
 
-    @unittest.skipUnless(os.name == "posix", "process probe is POSIX only")
-    def test_scratch_folders_of_dead_servers_are_removed(self):
+    @HAS_FLOCK
+    def test_the_scratch_folder_is_held_by_a_lock_until_close(self):
         memory = self.root / "memory"
         memory.mkdir()
-        dead = subprocess.Popen([sys.executable, "-c", "pass"])
-        dead.wait()
-        orphan = memory / f"pm-reset-{dead.pid}-abc"
-        alive = memory / f"pm-reset-{os.getpid()}-def"
-        other = memory / "pm-reset-notapid"
-        for folder in (orphan, alive, other):
-            folder.mkdir()
-            (folder / "seed.db").write_bytes(b"x")
         seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory)
         self.addCleanup(seeder.close)
         seeder.run(reseed)
-        self.assertFalse(orphan.exists())
-        self.assertTrue(alive.exists())
-        self.assertTrue(other.exists())
-        own = [p for p in memory.glob(f"pm-reset-{os.getpid()}-*") if p != alive]
-        self.assertEqual(len(own), 1)
+        (folder,) = memory.iterdir()
+        self.assertTrue(folder.name.startswith(f"pm-reset-held-{os.getpid()}-"))
+        self.assertFalse(lock_is_free(folder / "owner.lock"))
+        seeder.close()
+        self.assertEqual(list(memory.iterdir()), [])
+
+    @HAS_FLOCK
+    def test_a_lock_that_cannot_be_taken_puts_the_scratch_on_disk(self):
+        memory = self.root / "memory"
+        memory.mkdir()
+        seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory)
+        self.addCleanup(seeder.close)
+        refuse = mock.patch.object(
+            fast_reset.fcntl, "flock", side_effect=OSError("no locks here")
+        )
+        with refuse:
+            seeder.run(reseed)
+        self.assertEqual(list(memory.iterdir()), [])
+        self.assertTrue((self.root / "fallback" / "seed.db").exists())
 
     def test_close_releases_the_scratch_files(self):
         self.seeder.run(reseed)
@@ -331,5 +351,95 @@ class TestScratchSeeder(unittest.TestCase):
         self.seeder.close()  # idempotent
 
 
+# A process that takes the owner lock of a folder, says so, and waits.
+HOLDER = """
+import fcntl, os, sys, time
+fd = os.open(os.path.join(sys.argv[1], "owner.lock"), os.O_RDWR | os.O_CREAT)
+fcntl.flock(fd, fcntl.LOCK_EX)
+print("held", flush=True)
+time.sleep(600)
+"""
+
+
+def lock_is_free(path: Path) -> bool:
+    fd = os.open(path, os.O_RDWR)
+    try:
+        fast_reset.fcntl.flock(fd, fast_reset.fcntl.LOCK_EX | fast_reset.fcntl.LOCK_NB)
+    except BlockingIOError:
+        return False
+    finally:
+        os.close(fd)
+    return True
+
+
+@HAS_FLOCK
+class TestOrphanedScratch(unittest.TestCase):
+    """Only folders whose owner lock is free are orphans (#419)."""
+
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self.tmp.cleanup)
+        self.memory = Path(self.tmp.name)
+
+    def folder(self, name: str) -> Path:
+        folder = self.memory / name
+        folder.mkdir()
+        (folder / "seed.db").write_bytes(b"x")
+        return folder
+
+    def hold(self, folder: Path) -> subprocess.Popen:
+        holder = subprocess.Popen(
+            [sys.executable, "-c", HOLDER, str(folder)],
+            stdout=subprocess.PIPE,
+            text=True,
+        )
+        self.addCleanup(holder.wait)
+        self.addCleanup(holder.stdout.close)
+        self.addCleanup(holder.kill)
+        self.assertEqual(holder.stdout.readline().strip(), "held")
+        return holder
+
+    def test_a_folder_whose_owner_was_killed_is_removed(self):
+        folder = self.folder("pm-reset-held-123-abc")
+        holder = self.hold(folder)
+        holder.kill()
+        holder.wait()
+        fast_reset.remove_orphaned_scratch(self.memory)
+        self.assertFalse(folder.exists())
+
+    def test_a_held_folder_is_kept_whatever_pid_its_name_carries(self):
+        # Under --ipc=host the owner's pid is from another PID namespace: it
+        # does not exist here, but its lock does.
+        dead = subprocess.Popen([sys.executable, "-c", "pass"])
+        dead.wait()
+        folder = self.folder(f"pm-reset-held-{dead.pid}-abc")
+        self.hold(folder)
+        fast_reset.remove_orphaned_scratch(self.memory)
+        self.assertTrue(folder.exists())
+        self.assertTrue((folder / "seed.db").exists())
+
+    def test_folders_without_an_owner_lock_are_kept(self):
+        dead = subprocess.Popen([sys.executable, "-c", "pass"])
+        dead.wait()
+        legacy = self.folder(f"pm-reset-{dead.pid}-abc")
+        staging = self.folder(f"pm-reset-new-{dead.pid}-def")
+        unlocked = self.folder(f"pm-reset-held-{dead.pid}-ghi")
+        fast_reset.remove_orphaned_scratch(self.memory)
+        for folder in (legacy, staging, unlocked):
+            self.assertTrue(folder.exists(), folder.name)
+
+    def test_a_folder_held_by_another_seeder_in_this_process_is_kept(self):
+        api = types.SimpleNamespace(db=PromptDatabase(str(self.memory / "live.db")))
+        self.addCleanup(api.db.close_all)
+        root = self.memory / "shm"
+        root.mkdir()
+        other = ScratchSeeder(api, self.memory / "fallback", memory_root=root)
+        self.addCleanup(other.close)
+        other.run(reseed)
+        (folder,) = root.iterdir()
+        fast_reset.remove_orphaned_scratch(root)
+        self.assertTrue(folder.exists())
+
+
 if __name__ == "__main__":
     unittest.main()