fix: better locking

charmcraft/services/provider.py

@@ -17,7 +17,12 @@
 """Service class for creating providers."""
 from __future__ import annotations
 
-import atexit
+import contextlib
+import io
+from collections.abc import Generator
+
+from craft_application.models import BuildInfo
+from overrides import override
 
 try:
     import fcntl
@@ -27,17 +32,41 @@
 import pathlib
 from typing import cast
 
+import craft_application
 import craft_providers
 from craft_application import services
 from craft_cli import emit
 from craft_providers import bases
 
-from charmcraft import env
+from charmcraft import env, models
 
 
 class ProviderService(services.ProviderService):
     """Business logic for getting providers."""
 
+    def __init__(
+        self,
+        app: craft_application.AppMetadata,
+        services: craft_application.ServiceFactory,
+        *,
+        project: models.CharmcraftProject,
+        work_dir: pathlib.Path,
+        build_plan: list[BuildInfo],
+        provider_name: str | None = None,
+        install_snap: bool = True,
+    ) -> None:
+        super().__init__(
+            app,
+            services,
+            project=project,
+            work_dir=work_dir,
+            build_plan=build_plan,
+            provider_name=provider_name,
+            install_snap=install_snap,
+        )
+        self._cache_path = None
+        self._lock = None
+
     def setup(self) -> None:
         """Set up the provider service for Charmcraft."""
         super().setup()
@@ -64,23 +93,45 @@ def get_base(
 
         If no cache_path is included, adds one.
         """
-        cache_path = cast(pathlib.Path, kwargs.get("cache_path", env.get_host_shared_cache_path()))
-        our_lock = _maybe_lock_cache(cache_path)
+        self._cache_path = cast(
+            pathlib.Path, kwargs.get("cache_path", env.get_host_shared_cache_path())
+        )
+        self._lock = _maybe_lock_cache(self._cache_path)
 
         # Forward the shared cache path.
-        kwargs["cache_path"] = cache_path if our_lock else None
+        kwargs["cache_path"] = self._cache_path if self._lock else None
         return super().get_base(
             base_name,
             instance_name=instance_name,
             # craft-application annotation is incorrect
             **kwargs,  # type: ignore[arg-type]
         )
 
-
-def _maybe_lock_cache(path: pathlib.Path) -> bool:
+    @override
+    @contextlib.contextmanager
+    def instance(
+        self,
+        build_info: BuildInfo,
+        *,
+        work_dir: pathlib.Path,
+        allow_unstable: bool = True,
+        **kwargs: bool | str | None,
+    ) -> Generator[craft_providers.Executor, None, None]:
+        with super().instance(
+            build_info, work_dir=work_dir, allow_unstable=allow_unstable, **kwargs
+        ) as instance:
+            try:
+                yield instance
+            finally:
+                if fcntl is not None and self._lock:
+                    fcntl.flock(self._lock, fcntl.LOCK_UN)
+                    self._lock.close()
+
+
+def _maybe_lock_cache(path: pathlib.Path) -> io.TextIOBase | None:
     """Lock the cache so we only have one copy of Charmcraft using it at a time."""
     if fcntl is None:  # Don't lock on Windows - just don't cache.
-        return False
+        return None
     cache_lock_path = path / "charmcraft.lock"
 
     emit.trace("Attempting to lock the cache path")
@@ -92,14 +143,11 @@ def _maybe_lock_cache(path: pathlib.Path) -> bool:
         emit.progress(
             "Shared cache locked by another process; running without cache.", permanent=True
         )
-        return False
+        return None
     else:
         pid = str(os.getpid())
         lock_file.write(pid)
         lock_file.flush()
         os.fsync(lock_file.fileno())
         emit.trace(f"Cache path locked by this process ({pid})")
-        atexit.register(fcntl.flock, lock_file, fcntl.LOCK_UN)
-        atexit.register(cache_lock_path.unlink, missing_ok=True)
-
-        return True
+        return lock_file

pyproject.toml

@@ -330,6 +330,9 @@ lint.ignore = [
 # Allow Pydantic's `@validator` decorator to trigger class method treatment.
 classmethod-decorators = ["pydantic.validator"]
 
+[tool.ruff.lint.pydocstyle]
+ignore-decorators = ["overrides.overrides", "overrides.override", "typing.overload", "typing.override"]
+
 [tool.ruff.lint.per-file-ignores]
 "tests/**.py" = [  # Some things we want for the moin project are unnecessary in tests.
     "D",  # Ignore docstring rules in tests

tests/integration/services/test_provider.py

@@ -16,6 +16,7 @@
 """Integration tests for the provider service."""
 
 import pathlib
+import subprocess
 import sys
 
 import pytest
@@ -35,19 +36,25 @@ def test_lock_cache(
 ):
     cache_path = tmp_path / "cache"
     cache_path.mkdir()
+    lock_file = cache_path / "charmcraft.lock"
+    bash_lock_cmd = ["bash", "-c", f"flock -n {lock_file} true"]
     provider = service_factory.provider
     provider_kwargs = {
         "build_info": default_build_info,
         "work_dir": pathlib.Path(__file__).parent,
         "cache_path": cache_path,
     }
-    assert not (cache_path / "charmcraft.lock").exists()
+    assert not lock_file.exists()
 
     with provider.instance(**provider_kwargs):
         # Test that the cache lock gets created
-        assert (cache_path / "charmcraft.lock").is_file()
+        assert lock_file.is_file()
+        with pytest.raises(subprocess.CalledProcessError):
+            # Another process should not be able to lock the file.
+            subprocess.run(bash_lock_cmd, check=True)
 
-    (cache_path / "charmcraft.lock").write_text("Test that file isn't locked.")
+    # After exiting we should be able to lock the file.
+    subprocess.run(bash_lock_cmd, check=True)
 
 
 @pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
@@ -59,8 +66,16 @@ def test_locked_cache_no_cache(
 ):
     cache_path = tmp_path / "cache"
     cache_path.mkdir()
-    _maybe_lock_cache(cache_path)
-    assert (cache_path / "charmcraft.lock").exists()
+    lock_file = cache_path / "charmcraft.lock"
+
+    bash_lock_cmd = ["bash", "-c", f"flock -n {lock_file} true"]
+    # Check that we can lock the file from another process.
+    subprocess.run(bash_lock_cmd, check=True)
+    _ = _maybe_lock_cache(cache_path)
+    # And now we can't.
+    with pytest.raises(subprocess.CalledProcessError):
+        subprocess.run(bash_lock_cmd, check=True)
+
     provider = service_factory.provider
     provider_kwargs = {
         "build_info": default_build_info,
@@ -69,9 +84,15 @@ def test_locked_cache_no_cache(
     }
 
     with provider.instance(**provider_kwargs) as instance:
-        # Because we've already locked the cache, we don't get a subdirectory in the cache.
-        assert list(cache_path.iterdir()) == [cache_path / "charmcraft.lock"]
-
         # Create a file in the cache and ensure it's not visible in the outer fs
         instance.execute_run(["touch", "/root/.cache/cache_cached"])
+
+        # Because we've already locked the cache, we don't get a subdirectory in
+        # the cache, and thus the touch command inside there only affected the
+        # instance cache and not the shared cache.
+        assert list(cache_path.iterdir()) == [cache_path / "charmcraft.lock"]
+        emitter.assert_progress(
+            "Shared cache locked by another process; running without cache.", permanent=True
+        )
+
         assert not (tmp_path / "cache_cached").exists()