fix: lock shared cache directory

Locks the shared cache directory to prevent concurrency issues.

Fixes #1845

CRAFT-3313

charmcraft/services/provider.py

@@ -17,11 +17,18 @@
 """Service class for creating providers."""
 from __future__ import annotations
 
+import atexit
+try:
+    import fcntl
+except ImportError:  # Not available on Windows.
+    fcntl = None
 import os
 import pathlib
+from typing import cast
 
 import craft_providers
 from craft_application import services
+from craft_cli import emit
 from craft_providers import bases
 
 from charmcraft import env
@@ -56,12 +63,42 @@ 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)
+
         # Forward the shared cache path.
-        if "cache_path" not in kwargs:
-            kwargs["cache_path"] = env.get_host_shared_cache_path()
+        kwargs["cache_path"] = cache_path if our_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:
+    """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
+    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 False
+    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

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,49 @@
+# 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
+
+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
+
+
+def test_locks_cache(
+    service_factory: services.CharmcraftServiceFactory,
+    tmp_path: pathlib.Path,
+    default_build_info: BuildInfo,
+    emitter: RecordingEmitter,
+):
+    _maybe_lock_cache(tmp_path)
+    assert (tmp_path / "charmcraft.lock").exists()
+    provider = service_factory.provider
+    provider_kwargs = {
+        "build_info": default_build_info,
+        "work_dir": pathlib.Path(__file__),
+        "cache_path": tmp_path,
+    }
+
+    with provider.instance(**provider_kwargs) as instance:
+        # Because we've already locked the cache, we shouldn't see the lockfile.
+        lock_test = instance.execute_run(["test", "-f", "/root/.cache/charmcraft.lock"])
+        assert lock_test.returncode == 1
+
+        # Create a file in the cache and ensure it's not visible in the outer fs
+        instance.execute_run(["touch", "/root/.cache/cache_cached"])
+        assert not (tmp_path / "cache_cached").exists()

tests/unit/services/test_provider.py

@@ -15,12 +15,21 @@
 # For further info, check https://github.com/canonical/charmcraft
 """Unit tests for the provider service."""
 
+import fcntl
+import functools
 import pathlib
+from collections.abc import Iterator
+import platform
+import sys
+from unittest import mock
 
 import pytest
+from craft_cli.pytest_plugin import RecordingEmitter
 from craft_providers import bases
 
 from charmcraft import models, services
+from charmcraft.application.main import APP_METADATA
+from charmcraft.services.provider import _maybe_lock_cache
 
 
 @pytest.fixture
@@ -42,6 +51,17 @@ def provider_service(
     return service_factory.provider
 
 
+@pytest.fixture
+def mock_register(monkeypatch) -> Iterator[mock.Mock]:
+    register = mock.Mock()
+    monkeypatch.setattr("atexit.register", register)
+    yield register
+
+    # Call the exit hooks as if exiting the application.
+    for hook in register.mock_calls:
+        functools.partial(*hook.args)()
+
+
 @pytest.mark.parametrize(
     "base_name",
     [
@@ -76,3 +96,68 @@ def test_get_base_forwards_cache(
     )
 
     assert base._cache_path == fake_path / "cache"
+
+
+@pytest.mark.parametrize(
+    "base_name",
+    [
+        bases.BaseName("ubuntu", "20.04"),
+        bases.BaseName("ubuntu", "22.04"),
+        bases.BaseName("ubuntu", "24.04"),
+        bases.BaseName("ubuntu", "devel"),
+        bases.BaseName("almalinux", "9"),
+    ],
+)
+@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
+def test_get_base_no_cache_if_locked(
+    monkeypatch,
+    mock_register,
+    tmp_path: pathlib.Path,
+    base_name: bases.BaseName,
+    emitter: RecordingEmitter,
+):
+    cache_path = tmp_path / "cache"
+    cache_path.mkdir(exist_ok=True, parents=True)
+
+    # Make a new path object to work around caching the paths and thus getting the
+    # same file descriptor.
+    locked = _maybe_lock_cache(cache_path)
+    assert locked
+    new_cache_path = pathlib.Path(str(cache_path))
+    monkeypatch.setattr("charmcraft.env.get_host_shared_cache_path", lambda: new_cache_path)
+
+    # Can't use the fixture as pyfakefs doesn't handle locks.
+    provider_service = services.ProviderService(
+        app=APP_METADATA,
+        services=None,  # pyright: ignore[reportArgumentType]
+        project=None,  # pyright: ignore[reportArgumentType]
+        work_dir=tmp_path,
+        build_plan=[],
+    )
+
+    base = provider_service.get_base(
+        base_name=base_name,
+        instance_name="charmcraft-test-instance",
+    )
+
+    assert base._cache_path is None
+    emitter.assert_progress(
+        "Shared cache locked by another process; running without cache.",
+        permanent=True,
+    )
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
+def test_maybe_lock_cache_locks_single_lock(tmp_path: pathlib.Path, mock_register) -> None:
+    assert _maybe_lock_cache(tmp_path) is True
+    mock_register.assert_has_calls([mock.call(fcntl.flock, mock.ANY, fcntl.LOCK_UN)])
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
+def test_maybe_lock_cache_with_another_lock(tmp_path: pathlib.Path, mock_register) -> None:
+    assert _maybe_lock_cache(tmp_path) is True
+    assert _maybe_lock_cache(tmp_path) is False
+    assert mock_register.mock_calls == [
+        mock.call(fcntl.flock, mock.ANY, fcntl.LOCK_UN),
+        mock.call(mock.ANY, missing_ok=True),
+    ]