From e4c128f39888bfc5a0c0915745e81d5d2f365619 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Wed, 28 Oct 2026 15:39:38 -0700 Subject: [PATCH] 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 | 84 +++++++++++++++---- tests/test_browser_fast_reset.py | 140 +++++++++++++++++++++++++++---- 2 files changed, 192 insertions(+), 32 deletions(-) diff --git a/tests/browser/fast_reset.py b/tests/browser/fast_reset.py index c305c686..9706b13f 100644 --- a/tests/browser/fast_reset.py +++ b/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 -. +# Scratch folders are created as new--, locked, then +# renamed to held--. 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 diff --git a/tests/test_browser_fast_reset.py b/tests/test_browser_fast_reset.py index 6be9a54a..c23fae43 100644 --- a/tests/test_browser_fast_reset.py +++ b/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()