From 73f1d6e778068bfab4bd50a005cbd8356dd44409 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 23 Oct 2026 06:10:45 +0000 Subject: [PATCH 1/5] fix: install clang tools one hook process at a time pre-commit and prek split the files into batches and run the hook on each batch in parallel (require_serial: false). On the first run every process saw that the wheel was missing and ran pip install into the same environment at once, so the processes deleted or overwrote each other's files and ran half-written binaries: missing dist-info/WHEEL, 'Text file busy', 'Permission denied' on clang-format. Take an exclusive file lock on the hook's environment around the check and the install. The first process installs; the others wait, then find and use the installed wheel. --- cpp_linter_hooks/util.py | 81 ++++++++++++++++++++++ tests/test_util.py | 143 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 223 insertions(+), 1 deletion(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index dac3565..161419c 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -1,5 +1,7 @@ """Shared helpers for resolving and installing clang tool wheels.""" +import contextlib +import errno import json import logging import os @@ -207,6 +209,73 @@ def _find_installed_wheel(tool: str, version: str | None) -> tuple[Path, str] | return Path(path), installed +# Lock file in the hook's environment, so that hook processes running at the +# same time (pre-commit and prek run batches of files in parallel) install into +# the environment one at a time. +_INSTALL_LOCK_NAME = ".cpp-linter-hooks-install.lock" +_WINDOWS = os.name == "nt" + + +def _lock_file(fileno: int) -> None: + """Block until this process holds the exclusive lock on *fileno*.""" + if _WINDOWS: + import msvcrt + + while True: + try: + # Locks the first byte. LK_LOCK retries for about 10 seconds, + # then fails with EDEADLOCK while another process still holds it. + msvcrt.locking(fileno, msvcrt.LK_LOCK, 1) + except OSError as exc: + if exc.errno != errno.EDEADLOCK: + raise + else: + return + else: + import fcntl + + fcntl.flock(fileno, fcntl.LOCK_EX) + + +def _unlock_file(fileno: int) -> None: + """Release the lock taken by :func:`_lock_file`.""" + if _WINDOWS: + import msvcrt + + msvcrt.locking(fileno, msvcrt.LK_UNLCK, 1) + else: + import fcntl + + fcntl.flock(fileno, fcntl.LOCK_UN) + + +@contextlib.contextmanager +def _install_lock(): + """Hold a lock on this Python environment while checking for and installing tools. + + Without it, parallel hook processes all see that the tool is missing and run + ``pip install`` into the same directory at once, so one process deletes or + overwrites files another is still writing, or runs a binary another process + has not finished writing. The operating system releases the lock if the + process dies. If the lock file cannot be created, the hook runs unlocked. + """ + path = os.path.join(sys.prefix, _INSTALL_LOCK_NAME) + try: + fileno = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) + except OSError as exc: + LOG.debug("Could not open %s, installing without a lock: %s", path, exc) + yield + return + try: + _lock_file(fileno) + try: + yield + finally: + _unlock_file(fileno) + finally: + os.close(fileno) + + def _install_tool(tool: str, version: str) -> Path | None: """Install a tool using pip, logging output on failure.""" result = subprocess.run( @@ -233,7 +302,19 @@ def resolve_install_with_diagnostics( ``additional_dependencies`` are kept and the hook works offline. Otherwise the version is resolved dynamically from PyPI — no hardcoded list is maintained in-tree. + + The check and the install run under :func:`_install_lock`, so when several + hook processes start at once, one installs and the others wait and then use + the wheel it installed. """ + with _install_lock(): + return _resolve_install_locked(tool, version, verbose) + + +def _resolve_install_locked( + tool: str, version: str | None, verbose: bool +) -> tuple[Path | None, str | None]: + """Body of :func:`resolve_install_with_diagnostics`, run under the lock.""" installed_wheel = _find_installed_wheel(tool, version) if installed_wheel is not None: path, installed = installed_wheel diff --git a/tests/test_util.py b/tests/test_util.py index 11d0d0c..ed6a0cf 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -1,20 +1,26 @@ """Tests for cpp_linter_hooks.util -- dynamic PyPI version resolution.""" +import errno import json import logging import subprocess import sys import sysconfig +import textwrap +import types from pathlib import Path -from unittest.mock import patch +from unittest.mock import call, patch import pytest +from cpp_linter_hooks import util from cpp_linter_hooks.util import ( + _INSTALL_LOCK_NAME, _detect_installed_version, _env_paths, _find_installed_wheel, _get_pypi_versions, + _install_lock, _install_tool, _installed_wheel_version, _is_version_installed, @@ -912,3 +918,138 @@ def test_resolve_install_verbose_reports_installed_wheel(capsys): "Using installed clang-format Python wheel version 21.1.8" in capsys.readouterr().err ) + + +# ═══════════════════════════════════════════════════════════════════════ +# _install_lock +# ═══════════════════════════════════════════════════════════════════════ + +LOCK_WORKER = textwrap.dedent( + """ + import os, sys, time + sys.prefix = sys.argv[1] + from cpp_linter_hooks.util import _install_lock + + log = sys.argv[2] + with _install_lock(): + with open(log, "a") as f: + f.write("start\\n") + time.sleep(0.2) + with open(log, "a") as f: + f.write("end\\n") + """ +) + + +def test_install_lock_serializes_processes(tmp_path): + """Hook processes started at once take turns instead of installing together.""" + log = tmp_path / "log.txt" + workers = [ + subprocess.Popen( + [sys.executable, "-c", LOCK_WORKER, str(tmp_path), str(log)], + ) + for _ in range(3) + ] + for worker in workers: + assert worker.wait(timeout=60) == 0 + + assert log.read_text().split() == ["start", "end"] * 3 + assert (tmp_path / _INSTALL_LOCK_NAME).is_file() + + +def test_install_lock_can_be_taken_again(tmp_path, monkeypatch): + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with _install_lock(): + pass + with _install_lock(): + pass + + +def test_install_lock_without_writable_environment(tmp_path, monkeypatch, caplog): + monkeypatch.setattr(sys, "prefix", str(tmp_path / "missing")) + with ( + caplog.at_level(logging.DEBUG, logger="cpp_linter_hooks.util"), + _install_lock(), + ): + entered = True + assert entered + assert "installing without a lock" in caplog.text + + +def test_install_lock_unlocks_on_error(tmp_path, monkeypatch): + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with pytest.raises(RuntimeError), _install_lock(): + raise RuntimeError("pip crashed") + with _install_lock(): + pass + + +def _fake_msvcrt(locking): + return types.SimpleNamespace(LK_LOCK=1, LK_UNLCK=0, locking=locking) + + +def test_lock_file_windows_waits_while_locked(monkeypatch): + calls = [] + + def locking(fileno, mode, nbytes): + calls.append((fileno, mode, nbytes)) + # LK_LOCK gives up after about 10 seconds while another process holds it. + if len(calls) == 1: + raise OSError(errno.EDEADLOCK, "Resource deadlock avoided") + + monkeypatch.setattr(util, "_WINDOWS", True) + monkeypatch.setitem(sys.modules, "msvcrt", _fake_msvcrt(locking)) + util._lock_file(7) + util._unlock_file(7) + + assert calls == [(7, 1, 1), (7, 1, 1), (7, 0, 1)] + + +def test_lock_file_windows_other_error(monkeypatch): + def locking(fileno, mode, nbytes): + raise OSError(errno.EBADF, "Bad file descriptor") + + monkeypatch.setattr(util, "_WINDOWS", True) + monkeypatch.setitem(sys.modules, "msvcrt", _fake_msvcrt(locking)) + with pytest.raises(OSError, match="Bad file descriptor"): + util._lock_file(7) + + +def test_lock_file_posix(monkeypatch): + fake_fcntl = types.SimpleNamespace( + LOCK_EX=2, LOCK_UN=8, flock=lambda fileno, op: None + ) + monkeypatch.setattr(util, "_WINDOWS", False) + monkeypatch.setitem(sys.modules, "fcntl", fake_fcntl) + with patch.object(fake_fcntl, "flock") as flock: + util._lock_file(7) + util._unlock_file(7) + assert flock.call_args_list == [call(7, 2), call(7, 8)] + + +def test_resolve_install_runs_under_install_lock(): + events = [] + + class Lock: + def __enter__(self): + events.append("lock") + + def __exit__(self, *exc): + events.append("unlock") + + def install(tool, version): + events.append("install") + return Path("/venv/bin/clang-format") + + with ( + patch("cpp_linter_hooks.util._install_lock", return_value=Lock()), + patch("shutil.which", return_value=None), + patch( + "cpp_linter_hooks.util._get_pypi_versions", side_effect=_pypi_side_effect + ), + patch("cpp_linter_hooks.util._install_tool", side_effect=install), + ): + result = resolve_install_with_diagnostics("clang-format", "20") + + assert result == (Path("/venv/bin/clang-format"), None) + assert events == ["lock", "install", "unlock"] From 57a15d64d33dc6939a929a517938c65ae53474e9 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 23 Oct 2026 06:33:02 +0000 Subject: [PATCH 2/5] fix: run unlocked when the file system refuses locks Also compare against errno.EDEADLK: macOS has no errno.EDEADLOCK, and on Windows the two are the same number. --- cpp_linter_hooks/util.py | 30 ++++++++++++++++++------------ tests/test_util.py | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 161419c..ea9eeea 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -224,10 +224,12 @@ def _lock_file(fileno: int) -> None: while True: try: # Locks the first byte. LK_LOCK retries for about 10 seconds, - # then fails with EDEADLOCK while another process still holds it. + # then fails with EDEADLOCK (the same number as EDEADLK, which, + # unlike EDEADLOCK, every platform's errno module defines) + # while another process still holds it. msvcrt.locking(fileno, msvcrt.LK_LOCK, 1) except OSError as exc: - if exc.errno != errno.EDEADLOCK: + if exc.errno != errno.EDEADLK: raise else: return @@ -257,23 +259,27 @@ def _install_lock(): ``pip install`` into the same directory at once, so one process deletes or overwrites files another is still writing, or runs a binary another process has not finished writing. The operating system releases the lock if the - process dies. If the lock file cannot be created, the hook runs unlocked. + process dies. If the lock file cannot be created or locked (some network + file systems do not support locks), the hook runs unlocked, as before. """ path = os.path.join(sys.prefix, _INSTALL_LOCK_NAME) + fileno = None try: fileno = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) + _lock_file(fileno) except OSError as exc: - LOG.debug("Could not open %s, installing without a lock: %s", path, exc) - yield - return + LOG.debug("Could not lock %s, installing without a lock: %s", path, exc) + if fileno is not None: + os.close(fileno) + fileno = None try: - _lock_file(fileno) - try: - yield - finally: - _unlock_file(fileno) + yield finally: - os.close(fileno) + if fileno is not None: + try: + _unlock_file(fileno) + finally: + os.close(fileno) def _install_tool(tool: str, version: str) -> Path | None: diff --git a/tests/test_util.py b/tests/test_util.py index ed6a0cf..d26008f 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -3,6 +3,7 @@ import errno import json import logging +import os import subprocess import sys import sysconfig @@ -976,14 +977,45 @@ def test_install_lock_without_writable_environment(tmp_path, monkeypatch, caplog assert "installing without a lock" in caplog.text +def _crash(): + raise RuntimeError("pip crashed") + + def test_install_lock_unlocks_on_error(tmp_path, monkeypatch): monkeypatch.setattr(sys, "prefix", str(tmp_path)) with pytest.raises(RuntimeError), _install_lock(): - raise RuntimeError("pip crashed") + _crash() with _install_lock(): pass +def test_install_lock_falls_back_when_locking_fails(tmp_path, monkeypatch, caplog): + """File systems without lock support (e.g. some NFS mounts) run unlocked.""" + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + closed = [] + real_close = os.close + + def close(fileno): + closed.append(fileno) + real_close(fileno) + + with ( + patch( + "cpp_linter_hooks.util._lock_file", + side_effect=OSError(errno.ENOLCK, "No locks available"), + ), + patch("cpp_linter_hooks.util._unlock_file") as unlock, + patch("os.close", side_effect=close), + caplog.at_level(logging.DEBUG, logger="cpp_linter_hooks.util"), + _install_lock(), + ): + entered = True + assert entered + assert len(closed) == 1 + unlock.assert_not_called() + assert "No locks available" in caplog.text + + def _fake_msvcrt(locking): return types.SimpleNamespace(LK_LOCK=1, LK_UNLCK=0, locking=locking) @@ -995,7 +1027,7 @@ def locking(fileno, mode, nbytes): calls.append((fileno, mode, nbytes)) # LK_LOCK gives up after about 10 seconds while another process holds it. if len(calls) == 1: - raise OSError(errno.EDEADLOCK, "Resource deadlock avoided") + raise OSError(errno.EDEADLK, "Resource deadlock avoided") monkeypatch.setattr(util, "_WINDOWS", True) monkeypatch.setitem(sys.modules, "msvcrt", _fake_msvcrt(locking)) From e72fbd05c05cff0e8ee32874fce99bdcea931610 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 23 Oct 2026 07:09:26 +0000 Subject: [PATCH 3/5] refactor: take the install lock with filelock The hand-written fcntl/msvcrt lock needed platform branches, a Windows retry loop and their tests. filelock does the same with one call. --- AGENTS.md | 4 +- cpp_linter_hooks/util.py | 79 ++++++----------------------- pyproject.toml | 1 + tests/test_util.py | 105 +++++++-------------------------------- uv.lock | 6 ++- 5 files changed, 39 insertions(+), 156 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e5552dc..7bef9d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,8 +23,8 @@ A pre-commit hook repo that auto-installs and runs `clang-format` and `clang-tid | File | Purpose | |------|---------| -| `pyproject.toml` | Build config, deps (`pip>=26.1`), dev deps, entry points | -| `cpp_linter_hooks/util.py` | Installed-wheel check, PyPI version resolution + pip install (prefix matching, offline fallback) | +| `pyproject.toml` | Build config, deps (`pip>=26.1`, `filelock>=3.16`), dev deps, entry points | +| `cpp_linter_hooks/util.py` | Installed-wheel check, PyPI version resolution + pip install (prefix matching, offline fallback), all under a `filelock` lock on the hook environment so parallel hook processes install once | | `cpp_linter_hooks/clang_format.py` | clang-format wrapper (`--verbose`, `--dry-run`) | | `cpp_linter_hooks/clang_tidy.py` | clang-tidy wrapper (`--compile-commands`, missing-database policy, `--jobs`, `--fix`, `--all-lines`, `-quiet` + status-line filter, `--checks` repair, error hints) | | `cpp_linter_hooks/changed_lines.py` | Added line ranges per file from `git diff` (staged changes, or `PRE_COMMIT_FROM_REF...PRE_COMMIT_TO_REF`) | diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index ea9eeea..4f75a4b 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -1,7 +1,6 @@ """Shared helpers for resolving and installing clang tool wheels.""" import contextlib -import errno import json import logging import os @@ -15,6 +14,8 @@ from functools import lru_cache from pathlib import Path +from filelock import FileLock + LOG = logging.getLogger(__name__) @@ -209,77 +210,25 @@ def _find_installed_wheel(tool: str, version: str | None) -> tuple[Path, str] | return Path(path), installed -# Lock file in the hook's environment, so that hook processes running at the -# same time (pre-commit and prek run batches of files in parallel) install into -# the environment one at a time. -_INSTALL_LOCK_NAME = ".cpp-linter-hooks-install.lock" -_WINDOWS = os.name == "nt" - - -def _lock_file(fileno: int) -> None: - """Block until this process holds the exclusive lock on *fileno*.""" - if _WINDOWS: - import msvcrt - - while True: - try: - # Locks the first byte. LK_LOCK retries for about 10 seconds, - # then fails with EDEADLOCK (the same number as EDEADLK, which, - # unlike EDEADLOCK, every platform's errno module defines) - # while another process still holds it. - msvcrt.locking(fileno, msvcrt.LK_LOCK, 1) - except OSError as exc: - if exc.errno != errno.EDEADLK: - raise - else: - return - else: - import fcntl - - fcntl.flock(fileno, fcntl.LOCK_EX) - - -def _unlock_file(fileno: int) -> None: - """Release the lock taken by :func:`_lock_file`.""" - if _WINDOWS: - import msvcrt - - msvcrt.locking(fileno, msvcrt.LK_UNLCK, 1) - else: - import fcntl - - fcntl.flock(fileno, fcntl.LOCK_UN) - - @contextlib.contextmanager def _install_lock(): - """Hold a lock on this Python environment while checking for and installing tools. - - Without it, parallel hook processes all see that the tool is missing and run - ``pip install`` into the same directory at once, so one process deletes or - overwrites files another is still writing, or runs a binary another process - has not finished writing. The operating system releases the lock if the - process dies. If the lock file cannot be created or locked (some network - file systems do not support locks), the hook runs unlocked, as before. + """Let one hook process at a time check for and install tools in this environment. + + pre-commit and prek run the hook on batches of files in parallel. Without + the lock, every batch runs ``pip install`` into the same directory at once. + File systems without lock support (some NFS mounts) run unlocked, as before. """ - path = os.path.join(sys.prefix, _INSTALL_LOCK_NAME) - fileno = None + lock = FileLock(os.path.join(sys.prefix, ".cpp-linter-hooks-install.lock")) try: - fileno = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) - _lock_file(fileno) - except OSError as exc: - LOG.debug("Could not lock %s, installing without a lock: %s", path, exc) - if fileno is not None: - os.close(fileno) - fileno = None + lock.acquire() + except (OSError, NotImplementedError) as exc: + LOG.debug("Installing without a lock: %s", exc) + lock = None try: yield finally: - if fileno is not None: - try: - _unlock_file(fileno) - finally: - os.close(fileno) + if lock is not None: + lock.release() def _install_tool(tool: str, version: str) -> Path | None: diff --git a/pyproject.toml b/pyproject.toml index 09c542d..89225d7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ classifiers = [ "Topic :: Software Development :: Build Tools", ] dependencies = [ + "filelock>=3.16", # One hook process at a time installs clang tools "pip>=26.1", # Required to install clang-format and clang-tidy ] dynamic = ["version"] diff --git a/tests/test_util.py b/tests/test_util.py index d26008f..7464e0f 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -3,20 +3,16 @@ import errno import json import logging -import os import subprocess import sys import sysconfig import textwrap -import types from pathlib import Path -from unittest.mock import call, patch +from unittest.mock import patch import pytest -from cpp_linter_hooks import util from cpp_linter_hooks.util import ( - _INSTALL_LOCK_NAME, _detect_installed_version, _env_paths, _find_installed_wheel, @@ -955,108 +951,41 @@ def test_install_lock_serializes_processes(tmp_path): assert worker.wait(timeout=60) == 0 assert log.read_text().split() == ["start", "end"] * 3 - assert (tmp_path / _INSTALL_LOCK_NAME).is_file() + assert (tmp_path / ".cpp-linter-hooks-install.lock").is_file() -def test_install_lock_can_be_taken_again(tmp_path, monkeypatch): +def test_install_lock_unlocks_on_error(tmp_path, monkeypatch): monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with pytest.raises(RuntimeError), _install_lock(): + _crash() with _install_lock(): pass - with _install_lock(): - pass - - -def test_install_lock_without_writable_environment(tmp_path, monkeypatch, caplog): - monkeypatch.setattr(sys, "prefix", str(tmp_path / "missing")) - with ( - caplog.at_level(logging.DEBUG, logger="cpp_linter_hooks.util"), - _install_lock(), - ): - entered = True - assert entered - assert "installing without a lock" in caplog.text def _crash(): raise RuntimeError("pip crashed") -def test_install_lock_unlocks_on_error(tmp_path, monkeypatch): - monkeypatch.setattr(sys, "prefix", str(tmp_path)) - with pytest.raises(RuntimeError), _install_lock(): - _crash() - with _install_lock(): - pass - - -def test_install_lock_falls_back_when_locking_fails(tmp_path, monkeypatch, caplog): +@pytest.mark.parametrize( + "error", + [ + OSError(errno.ENOLCK, "No locks available"), + NotImplementedError("FileSystem does not appear to support flock"), + ], +) +def test_install_lock_runs_unlocked_without_lock_support( + tmp_path, monkeypatch, caplog, error +): """File systems without lock support (e.g. some NFS mounts) run unlocked.""" monkeypatch.setattr(sys, "prefix", str(tmp_path)) - closed = [] - real_close = os.close - - def close(fileno): - closed.append(fileno) - real_close(fileno) - with ( - patch( - "cpp_linter_hooks.util._lock_file", - side_effect=OSError(errno.ENOLCK, "No locks available"), - ), - patch("cpp_linter_hooks.util._unlock_file") as unlock, - patch("os.close", side_effect=close), + patch("cpp_linter_hooks.util.FileLock.acquire", side_effect=error), caplog.at_level(logging.DEBUG, logger="cpp_linter_hooks.util"), _install_lock(), ): entered = True assert entered - assert len(closed) == 1 - unlock.assert_not_called() - assert "No locks available" in caplog.text - - -def _fake_msvcrt(locking): - return types.SimpleNamespace(LK_LOCK=1, LK_UNLCK=0, locking=locking) - - -def test_lock_file_windows_waits_while_locked(monkeypatch): - calls = [] - - def locking(fileno, mode, nbytes): - calls.append((fileno, mode, nbytes)) - # LK_LOCK gives up after about 10 seconds while another process holds it. - if len(calls) == 1: - raise OSError(errno.EDEADLK, "Resource deadlock avoided") - - monkeypatch.setattr(util, "_WINDOWS", True) - monkeypatch.setitem(sys.modules, "msvcrt", _fake_msvcrt(locking)) - util._lock_file(7) - util._unlock_file(7) - - assert calls == [(7, 1, 1), (7, 1, 1), (7, 0, 1)] - - -def test_lock_file_windows_other_error(monkeypatch): - def locking(fileno, mode, nbytes): - raise OSError(errno.EBADF, "Bad file descriptor") - - monkeypatch.setattr(util, "_WINDOWS", True) - monkeypatch.setitem(sys.modules, "msvcrt", _fake_msvcrt(locking)) - with pytest.raises(OSError, match="Bad file descriptor"): - util._lock_file(7) - - -def test_lock_file_posix(monkeypatch): - fake_fcntl = types.SimpleNamespace( - LOCK_EX=2, LOCK_UN=8, flock=lambda fileno, op: None - ) - monkeypatch.setattr(util, "_WINDOWS", False) - monkeypatch.setitem(sys.modules, "fcntl", fake_fcntl) - with patch.object(fake_fcntl, "flock") as flock: - util._lock_file(7) - util._unlock_file(7) - assert flock.call_args_list == [call(7, 2), call(7, 8)] + assert "Installing without a lock" in caplog.text def test_resolve_install_runs_under_install_lock(): diff --git a/uv.lock b/uv.lock index 27d611b..6c34416 100644 --- a/uv.lock +++ b/uv.lock @@ -152,6 +152,7 @@ wheels = [ name = "cpp-linter-hooks" source = { editable = "." } dependencies = [ + { name = "filelock" }, { name = "pip" }, ] @@ -164,7 +165,10 @@ dev = [ ] [package.metadata] -requires-dist = [{ name = "pip", specifier = ">=26.1" }] +requires-dist = [ + { name = "filelock", specifier = ">=3.16" }, + { name = "pip", specifier = ">=26.1" }, +] [package.metadata.requires-dev] dev = [ From 630114de13a5066807ed1c41b4f9958d82b56e8c Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 23 Oct 2026 07:23:50 +0000 Subject: [PATCH 4/5] perf: take the install lock only when a tool must be installed Importing filelock pulls in asyncio and sqlite3 (about 70 ms), which CodSpeed showed as a 38% slowdown on every hook run. Check for the installed wheel first without the lock, and import filelock and lock only when installing. A wheel counts as installed only once pip has written its RECORD file, so another process's unfinished install is never used. --- cpp_linter_hooks/util.py | 53 +++++++++++++++++++++++----------------- tests/test_util.py | 52 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 79 insertions(+), 26 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 4f75a4b..1b0dbe5 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -14,8 +14,6 @@ from functools import lru_cache from pathlib import Path -from filelock import FileLock - LOG = logging.getLogger(__name__) @@ -165,7 +163,9 @@ def _installed_wheel_version(tool: str) -> str | None: installed on an earlier run) into the hook's own environment. The version comes from the wheel's ``-.dist-info`` directory name, which needs neither the network nor a subprocess, and avoids parsing the large - METADATA file the way ``importlib.metadata.version`` does. + METADATA file the way ``importlib.metadata.version`` does. A wheel counts + only once its ``RECORD`` file exists: pip writes it last, so a wheel another + hook process is still installing is not used half-written. """ prefix = f"{re.sub(r'[-_.]+', '_', tool).lower()}-" suffix = ".dist-info" @@ -178,7 +178,11 @@ def _installed_wheel_version(tool: str) -> str | None: continue for name in names: lower = name.lower() - if lower.startswith(prefix) and lower.endswith(suffix): + if ( + lower.startswith(prefix) + and lower.endswith(suffix) + and os.path.isfile(os.path.join(site_dir, name, "RECORD")) + ): return name[len(prefix) : -len(suffix)] return None @@ -212,12 +216,16 @@ def _find_installed_wheel(tool: str, version: str | None) -> tuple[Path, str] | @contextlib.contextmanager def _install_lock(): - """Let one hook process at a time check for and install tools in this environment. + """Let one hook process at a time install tools into this environment. pre-commit and prek run the hook on batches of files in parallel. Without the lock, every batch runs ``pip install`` into the same directory at once. File systems without lock support (some NFS mounts) run unlocked, as before. + filelock is imported here because it is only needed to install, and + importing it takes longer than a whole hook run that finds the tool. """ + from filelock import FileLock + lock = FileLock(os.path.join(sys.prefix, ".cpp-linter-hooks-install.lock")) try: lock.acquire() @@ -258,28 +266,29 @@ def resolve_install_with_diagnostics( Otherwise the version is resolved dynamically from PyPI — no hardcoded list is maintained in-tree. - The check and the install run under :func:`_install_lock`, so when several - hook processes start at once, one installs and the others wait and then use - the wheel it installed. + Installing runs under :func:`_install_lock`, so when several hook processes + start at once, one installs and the others wait and then use its wheel. """ - with _install_lock(): - return _resolve_install_locked(tool, version, verbose) + installed_wheel = _find_installed_wheel(tool, version) + if installed_wheel is None: + with _install_lock(): + # Another hook process may have installed it while this one waited. + installed_wheel = _find_installed_wheel(tool, version) + if installed_wheel is None: + return _resolve_and_install(tool, version, verbose) + path, installed = installed_wheel + if verbose: + print( + f"Using installed {tool} Python wheel version {installed}", + file=sys.stderr, + ) + return path, None -def _resolve_install_locked( +def _resolve_and_install( tool: str, version: str | None, verbose: bool ) -> tuple[Path | None, str | None]: - """Body of :func:`resolve_install_with_diagnostics`, run under the lock.""" - installed_wheel = _find_installed_wheel(tool, version) - if installed_wheel is not None: - path, installed = installed_wheel - if verbose: - print( - f"Using installed {tool} Python wheel version {installed}", - file=sys.stderr, - ) - return path, None - + """Resolve *version* on PyPI and install it, unless it is already on PATH.""" user_version, error = _resolve_version_from_pypi(tool, version) if error is not None: return None, error diff --git a/tests/test_util.py b/tests/test_util.py index 7464e0f..9ce9962 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -743,6 +743,12 @@ def test_env_paths_match_sysconfig(): assert _env_paths() == sysconfig.get_paths() +def _installed(dist_info: Path) -> None: + """Create a dist-info directory the way pip leaves it after an install.""" + dist_info.mkdir(parents=True) + (dist_info / "RECORD").touch() + + def test_installed_wheel_version_reads_dist_info_name(tmp_path): for name in ( "clang_tidy_extra-1.0.dist-info", @@ -750,7 +756,7 @@ def test_installed_wheel_version_reads_dist_info_name(tmp_path): "clang_tidy", "Clang_Tidy-21.1.6.dist-info", ): - (tmp_path / name).mkdir() + _installed(tmp_path / name) with patch("cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path)): assert _installed_wheel_version("clang-tidy") == "21.1.6" assert _installed_wheel_version("clang-format") == "22.1.8" @@ -759,14 +765,21 @@ def test_installed_wheel_version_reads_dist_info_name(tmp_path): def test_installed_wheel_version_checks_platlib(tmp_path): purelib, platlib = tmp_path / "purelib", tmp_path / "platlib" purelib.mkdir() - (platlib / "clang_tidy-21.1.6.dist-info").mkdir(parents=True) + _installed(platlib / "clang_tidy-21.1.6.dist-info") paths = {"purelib": str(purelib), "platlib": str(platlib), "scripts": ""} with patch("cpp_linter_hooks.util._env_paths", return_value=paths): assert _installed_wheel_version("clang-tidy") == "21.1.6" -def test_installed_wheel_version_missing_distribution(tmp_path): +def test_installed_wheel_version_ignores_unfinished_install(tmp_path): + """pip writes RECORD last; without it another process is still installing.""" (tmp_path / "clang_format-22.1.8.dist-info").mkdir() + with patch("cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path)): + assert _installed_wheel_version("clang-format") is None + + +def test_installed_wheel_version_missing_distribution(tmp_path): + _installed(tmp_path / "clang_format-22.1.8.dist-info") with patch("cpp_linter_hooks.util._env_paths", return_value=_paths(tmp_path)): assert _installed_wheel_version("clang-tidy") is None with patch( @@ -979,7 +992,7 @@ def test_install_lock_runs_unlocked_without_lock_support( """File systems without lock support (e.g. some NFS mounts) run unlocked.""" monkeypatch.setattr(sys, "prefix", str(tmp_path)) with ( - patch("cpp_linter_hooks.util.FileLock.acquire", side_effect=error), + patch("filelock.FileLock.acquire", side_effect=error), caplog.at_level(logging.DEBUG, logger="cpp_linter_hooks.util"), _install_lock(), ): @@ -1014,3 +1027,34 @@ def install(tool, version): assert result == (Path("/venv/bin/clang-format"), None) assert events == ["lock", "install", "unlock"] + + +def test_resolve_install_skips_lock_when_installed(): + with ( + patch( + "cpp_linter_hooks.util._find_installed_wheel", + return_value=(Path("/venv/bin/clang-format"), "20.1.8"), + ), + patch("cpp_linter_hooks.util._install_lock") as lock, + ): + result = resolve_install_with_diagnostics("clang-format", "20") + + assert result == (Path("/venv/bin/clang-format"), None) + lock.assert_not_called() + + +def test_resolve_install_uses_wheel_installed_while_waiting(): + """Another hook process installed the wheel while this one waited for the lock.""" + with ( + patch( + "cpp_linter_hooks.util._find_installed_wheel", + side_effect=[None, (Path("/venv/bin/clang-format"), "20.1.8")], + ), + patch("cpp_linter_hooks.util._get_pypi_versions") as pypi, + patch("cpp_linter_hooks.util._install_tool") as install, + ): + result = resolve_install_with_diagnostics("clang-format", "20") + + assert result == (Path("/venv/bin/clang-format"), None) + pypi.assert_not_called() + install.assert_not_called() From 57655da2b0aac595f612aa6ad04d9db067581b6e Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Fri, 23 Oct 2026 07:32:22 +0000 Subject: [PATCH 5/5] fix: stop waiting for the install lock after 5 minutes filelock 3.x (the release Python 3.10 gets) keeps retrying when the file system refuses the lock, and a stale soft lock file from a crashed process is never freed. After 5 minutes the hook installs unlocked. --- cpp_linter_hooks/util.py | 15 +++++++++++---- tests/test_util.py | 12 ++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 1b0dbe5..d129cb2 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -214,13 +214,20 @@ def _find_installed_wheel(tool: str, version: str | None) -> tuple[Path, str] | return Path(path), installed +# Seconds to wait for another hook process to finish installing. +_INSTALL_LOCK_TIMEOUT = 300 + + @contextlib.contextmanager def _install_lock(): """Let one hook process at a time install tools into this environment. pre-commit and prek run the hook on batches of files in parallel. Without the lock, every batch runs ``pip install`` into the same directory at once. - File systems without lock support (some NFS mounts) run unlocked, as before. + If the lock is not free within 5 minutes, or the file system does not support + locks (some NFS mounts), the hook runs unlocked, as before. The limit also + covers filelock 3.x, which keeps retrying when the file system refuses the + lock, and a stale lock file left by a crashed process. filelock is imported here because it is only needed to install, and importing it takes longer than a whole hook run that finds the tool. """ @@ -228,9 +235,9 @@ def _install_lock(): lock = FileLock(os.path.join(sys.prefix, ".cpp-linter-hooks-install.lock")) try: - lock.acquire() - except (OSError, NotImplementedError) as exc: - LOG.debug("Installing without a lock: %s", exc) + lock.acquire(timeout=_INSTALL_LOCK_TIMEOUT) + except (OSError, NotImplementedError) as exc: # filelock.Timeout is an OSError + LOG.warning("Installing without a lock: %s", exc) lock = None try: yield diff --git a/tests/test_util.py b/tests/test_util.py index 9ce9962..563141b 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -984,6 +984,7 @@ def _crash(): [ OSError(errno.ENOLCK, "No locks available"), NotImplementedError("FileSystem does not appear to support flock"), + TimeoutError("The file lock could not be acquired"), ], ) def test_install_lock_runs_unlocked_without_lock_support( @@ -1001,6 +1002,17 @@ def test_install_lock_runs_unlocked_without_lock_support( assert "Installing without a lock" in caplog.text +def test_install_lock_waits_a_limited_time(tmp_path, monkeypatch): + monkeypatch.setattr(sys, "prefix", str(tmp_path)) + with ( + patch("filelock.FileLock.acquire") as acquire, + patch("filelock.FileLock.release"), + _install_lock(), + ): + pass + acquire.assert_called_once_with(timeout=300) + + def test_resolve_install_runs_under_install_lock(): events = []