diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index 7b74f7be1..31def57cc 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -71,6 +71,24 @@ def cache_lock( return _announced_lock(cache_dir.with_name(f".{cache_dir.name}.lock"), shared, cache_dir) +def _wait_message(lock_path: Path, shared: bool, cache_dir: Path) -> str: + """Who to wait for, and, for a refresh, how not to. + + The bare "another run holds it" named neither the lock nor its holder, + and the build waited with no limit. + """ + message = ( + f"waiting for {cache_dir}: another run holds {lock_path} " + f"(lsof {lock_path} names it)" + ) + if not shared: + message += ( + ". generate_pack.py --no-refresh-data, or --offline there and in" + " pipeline.py, builds from the cache as it is without waiting" + ) + return message + + @contextlib.contextmanager def _announced_lock(lock_path: Path, shared: bool, cache_dir: Path) -> Iterator[None]: """file_lock, saying so when it has to wait. @@ -90,7 +108,7 @@ def _announced_lock(lock_path: Path, shared: bool, cache_dir: Path) -> Iterator[ try: fcntl.flock(handle, mode | fcntl.LOCK_NB) except BlockingIOError: - log.warning("waiting for %s: another run holds it", cache_dir) + log.warning(_wait_message(lock_path, shared, cache_dir)) fcntl.flock(handle, mode) try: yield diff --git a/tests/test_refresh_data_dirs.py b/tests/test_refresh_data_dirs.py index 42f21d122..dc38143ac 100644 --- a/tests/test_refresh_data_dirs.py +++ b/tests/test_refresh_data_dirs.py @@ -118,6 +118,35 @@ class RefreshConcurrency(unittest.TestCase): self.assertTrue(finished, "the refresh waited for a reader") self.assertEqual(result, [False]) + def test_a_waiting_refresh_names_the_lock_and_the_way_around(self): + """"waiting for data/dolphin-sys: another run holds it" named + neither the lock nor its holder, and the build waited unbounded.""" + import fcntl # noqa: PLC0415 + + self.cache.mkdir(parents=True) + lock_path = self.cache.with_name(".pak.lock") + entered = threading.Event() + + def refresh() -> None: + with rdd.cache_lock(self.cache): + entered.set() + + with open(lock_path, "a") as reader, self.assertLogs(rdd.log, "WARNING") as logs: + fcntl.flock(reader, fcntl.LOCK_SH) + worker = threading.Thread(target=refresh, daemon=True) + worker.start() + for _ in range(200): + if logs.records: + break + worker.join(timeout=0.05) + self.assertFalse(entered.is_set()) + fcntl.flock(reader, fcntl.LOCK_UN) + worker.join(timeout=20) + self.assertTrue(entered.is_set()) + message = logs.records[0].getMessage() + self.assertIn(f"lsof {lock_path}", message) + self.assertIn("--no-refresh-data", message) + def test_version_file_is_never_left_truncated(self): Path(self.versions).parent.mkdir(parents=True) Path(self.versions).write_text(json.dumps({"keep": {"sha": "1"}}))