fix: lock shared cache directory (#1888)

Locks the shared cache directory to prevent concurrency issues.

Fixes #1845

CRAFT-3313

.github/workflows/tests.yaml

@@ -80,6 +80,9 @@ jobs:
         run: |
           sudo apt update
           sudo apt install -y python3-pip python3-setuptools python3-wheel python3-venv libapt-pkg-dev
+      - name: Setup LXD
+        uses: canonical/[email protected]
+        if: ${{ runner.os == 'Linux' }}
       - name: Install skopeo (mac)
         # This is only necessary for Linux until skopeo >= 1.11 is in repos.
         # Once we're running on Noble, we can get skopeo from apt.

charmcraft/services/provider.py

@@ -17,19 +17,55 @@
 """Service class for creating providers."""
 from __future__ import annotations
 
+import contextlib
+import io
+from collections.abc import Generator
+
+from craft_application.models import BuildInfo
+
+try:
+    import fcntl
+except ModuleNotFoundError:  # Not available on Windows.
+    fcntl = None  # type: ignore[assignment]
 import os
 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: pathlib.Path | None = None
+        self._lock: io.TextIOBase | None = None
+
     def setup(self) -> None:
         """Set up the provider service for Charmcraft."""
         super().setup()
@@ -56,12 +92,61 @@ def get_base(
 
         If no cache_path is included, adds one.
         """
+        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.
-        if "cache_path" not in kwargs:
-            kwargs["cache_path"] = env.get_host_shared_cache_path()
+        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]
         )
+
+    @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]:
+        """Instance override for Charmcraft."""
+        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 None
+    cache_lock_path = path / "charmcraft.lock"
+
+    emit.trace("Attempting to lock the cache path")
+    lock_file = cache_lock_path.open("w+")
+    try:
+        # Exclusive lock, but non-blocking.
+        fcntl.flock(lock_file, fcntl.LOCK_EX | fcntl.LOCK_NB)
+    except OSError:
+        emit.progress(
+            "Shared cache locked by another process; running without cache.", permanent=True
+        )
+        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})")
+        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/conftest.py

@@ -109,16 +109,19 @@ def service_factory(
 
 
 @pytest.fixture
-def default_build_plan():
+def default_build_info() -> models.BuildInfo:
     arch = util.get_host_architecture()
-    return [
-        models.BuildInfo(
-            base=bases.BaseName("ubuntu", "22.04"),
-            build_on=arch,
-            build_for="arm64",
-            platform="distro-1-test64",
-        )
-    ]
+    return models.BuildInfo(
+        base=bases.BaseName("ubuntu", "22.04"),
+        build_on=arch,
+        build_for="arm64",
+        platform="distro-1-test64",
+    )
+
+
+@pytest.fixture
+def default_build_plan(default_build_info: models.BuildInfo):
+    return [default_build_info]
 
 
 @pytest.fixture

tests/integration/services/conftest.py

@@ -14,31 +14,18 @@
 #
 # For further info, check https://github.com/canonical/charmcraft
 """Configuration for services integration tests."""
-import contextlib
-import sys
 
-import pyfakefs.fake_filesystem
 import pytest
 
 from charmcraft import services
 from charmcraft.application.main import APP_METADATA, Charmcraft
 
 
 @pytest.fixture
-def service_factory(
-    fs: pyfakefs.fake_filesystem.FakeFilesystem, fake_path, simple_charm
-) -> services.CharmcraftServiceFactory:
-    fake_project_dir = fake_path / "project"
+def service_factory(simple_charm, new_path) -> services.CharmcraftServiceFactory:
+    fake_project_dir = new_path / "project"
     fake_project_dir.mkdir()
 
-    # Allow access to the real venv library path.
-    # This is necessary because certifi lazy-loads the certificate file.
-    for python_path in sys.path:
-        if not python_path:
-            continue
-        with contextlib.suppress(OSError):
-            fs.add_real_directory(python_path)
-
     factory = services.CharmcraftServiceFactory(app=APP_METADATA)
 
     app = Charmcraft(app=APP_METADATA, services=factory)

tests/integration/services/test_package.py

@@ -14,7 +14,6 @@
 #
 # For further info, check https://github.com/canonical/charmcraft
 """Tests for package service."""
-import contextlib
 import datetime
 import pathlib
 
@@ -28,8 +27,8 @@
 
 
 @pytest.fixture
-def package_service(fake_path, service_factory, default_build_plan):
-    fake_project_dir = fake_path
+def package_service(new_path: pathlib.Path, service_factory, default_build_plan):
+    fake_project_dir = new_path
     svc = services.PackageService(
         app=APP_METADATA,
         project=service_factory.project,
@@ -49,12 +48,10 @@ def package_service(fake_path, service_factory, default_build_plan):
     ],
 )
 @freezegun.freeze_time(datetime.datetime(2020, 3, 14, 0, 0, 0, tzinfo=datetime.timezone.utc))
-def test_write_metadata(monkeypatch, fs, package_service, project_path):
+def test_write_metadata(monkeypatch, new_path, package_service, project_path):
     monkeypatch.setattr(charmcraft, "__version__", "3.0-test-version")
-    with contextlib.suppress(FileExistsError):
-        fs.add_real_directory(project_path)
-    test_prime_dir = pathlib.Path("/prime")
-    fs.create_dir(test_prime_dir)
+    test_prime_dir = new_path / "prime"
+    test_prime_dir.mkdir()
     expected_prime_dir = project_path / "prime"
 
     project = models.CharmcraftProject.from_yaml_file(project_path / "project" / "charmcraft.yaml")
@@ -75,23 +72,21 @@ def test_write_metadata(monkeypatch, fs, package_service, project_path):
     ],
 )
 @freezegun.freeze_time(datetime.datetime(2020, 3, 14, 0, 0, 0, tzinfo=datetime.timezone.utc))
-def test_overwrite_metadata(monkeypatch, fs, package_service, project_path):
+def test_overwrite_metadata(monkeypatch, new_path, package_service, project_path):
     """Test that the metadata file gets rewritten for a charm.
 
     Regression test for https://github.com/canonical/charmcraft/issues/1654
     """
     monkeypatch.setattr(charmcraft, "__version__", "3.0-test-version")
-    with contextlib.suppress(FileExistsError):
-        fs.add_real_directory(project_path)
-    test_prime_dir = pathlib.Path("/prime")
-    fs.create_dir(test_prime_dir)
+    test_prime_dir = new_path / "prime"
+    test_prime_dir.mkdir()
     expected_prime_dir = project_path / "prime"
 
     project = models.CharmcraftProject.from_yaml_file(project_path / "project" / "charmcraft.yaml")
     project._started_at = datetime.datetime.now(tz=datetime.timezone.utc)
     package_service._project = project
 
-    fs.create_file(test_prime_dir / const.METADATA_FILENAME, contents="INVALID!!")
+    (test_prime_dir / const.METADATA_FILENAME).write_text("INVALID!!")
 
     package_service.write_metadata(test_prime_dir)
 
@@ -100,20 +95,18 @@ def test_overwrite_metadata(monkeypatch, fs, package_service, project_path):
 
 
 @freezegun.freeze_time(datetime.datetime(2020, 3, 14, 0, 0, 0, tzinfo=datetime.timezone.utc))
-def test_no_overwrite_reactive_metadata(monkeypatch, fs, package_service):
+def test_no_overwrite_reactive_metadata(monkeypatch, new_path, package_service):
     """Test that the metadata file doesn't get overwritten for a reactive charm..
 
     Regression test for https://github.com/canonical/charmcraft/issues/1654
     """
     monkeypatch.setattr(charmcraft, "__version__", "3.0-test-version")
     project_path = pathlib.Path(__file__).parent / "sample_projects" / "basic-reactive"
-    with contextlib.suppress(FileExistsError):
-        fs.add_real_directory(project_path)
-    test_prime_dir = pathlib.Path("/prime")
-    fs.create_dir(test_prime_dir)
-    test_stage_dir = pathlib.Path("/stage")
-    fs.create_dir(test_stage_dir)
-    fs.create_file(test_stage_dir / const.METADATA_FILENAME, contents="INVALID!!")
+    test_prime_dir = new_path / "prime"
+    test_prime_dir.mkdir()
+    test_stage_dir = new_path / "stage"
+    test_stage_dir.mkdir()
+    (test_stage_dir / const.METADATA_FILENAME).write_text("INVALID!!")
 
     project = models.CharmcraftProject.from_yaml_file(project_path / "project" / "charmcraft.yaml")
     project._started_at = datetime.datetime.now(tz=datetime.timezone.utc)

tests/integration/services/test_provider.py

@@ -0,0 +1,103 @@
+# Copyright 2024 Canonical Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# For further info, check https://github.com/canonical/charmcraft
+"""Integration tests for the provider service."""
+
+import pathlib
+import shutil
+import subprocess
+import sys
+
+import pytest
+from craft_application.models import BuildInfo
+from craft_cli.pytest_plugin import RecordingEmitter
+
+from charmcraft import services
+from charmcraft.services.provider import _maybe_lock_cache
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
+def test_lock_cache(
+    service_factory: services.CharmcraftServiceFactory,
+    tmp_path: pathlib.Path,
+    default_build_info: BuildInfo,
+    emitter: RecordingEmitter,
+):
+    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"] if shutil.which("flock") else None
+    provider = service_factory.provider
+    provider_kwargs = {
+        "build_info": default_build_info,
+        "work_dir": pathlib.Path(__file__).parent,
+        "cache_path": cache_path,
+    }
+    assert not lock_file.exists()
+
+    with provider.instance(**provider_kwargs):
+        # Test that the cache lock gets created
+        assert lock_file.is_file()
+        if bash_lock_cmd:
+            with pytest.raises(subprocess.CalledProcessError):
+                # Another process should not be able to lock the file.
+                subprocess.run(bash_lock_cmd, check=True)
+
+    # After exiting we should be able to lock the file.
+    if bash_lock_cmd:
+        subprocess.run(bash_lock_cmd, check=True)
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
+def test_locked_cache_no_cache(
+    service_factory: services.CharmcraftServiceFactory,
+    tmp_path: pathlib.Path,
+    default_build_info: BuildInfo,
+    emitter: RecordingEmitter,
+):
+    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"] if shutil.which("flock") else None
+    # Check that we can lock the file from another process.
+    if bash_lock_cmd:
+        subprocess.run(bash_lock_cmd, check=True)
+    _ = _maybe_lock_cache(cache_path)
+    # And now we can't.
+    if bash_lock_cmd:
+        with pytest.raises(subprocess.CalledProcessError):
+            subprocess.run(bash_lock_cmd, check=True)
+
+    provider = service_factory.provider
+    provider_kwargs = {
+        "build_info": default_build_info,
+        "work_dir": pathlib.Path(__file__).parent,
+        "cache_path": cache_path,
+    }
+
+    with provider.instance(**provider_kwargs) as instance:
+        # 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()