From 2d9d3df1c58835714a8493346bd3e5d1f50c6fe4 Mon Sep 17 00:00:00 2001
From: Alex Lowe <alex.lowe@canonical.com>
Date: Mon, 9 Sep 2024 16:20:41 -0400
Subject: [PATCH 1/4] fix: lock shared cache directory

Locks the shared cache directory to prevent concurrency issues.

Fixes #1845

CRAFT-3313
---
 .github/workflows/tests.yaml                |  5 ++
 charmcraft/services/provider.py             | 42 +++++++++-
 tests/conftest.py                           | 21 ++---
 tests/integration/services/conftest.py      | 17 +---
 tests/integration/services/test_package.py  | 37 ++++-----
 tests/integration/services/test_provider.py | 52 ++++++++++++
 tests/unit/services/test_provider.py        | 88 +++++++++++++++++++++
 7 files changed, 214 insertions(+), 48 deletions(-)
 create mode 100644 tests/integration/services/test_provider.py

diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
index 33f2ae18f..ac9498234 100644
--- a/.github/workflows/tests.yaml
+++ b/.github/workflows/tests.yaml
@@ -80,6 +80,11 @@ 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/setup-lxd@v0.1.1
+        with:
+          channel: latest/stable
+        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.
diff --git a/charmcraft/services/provider.py b/charmcraft/services/provider.py
index cefcb9ad9..951cca117 100644
--- a/charmcraft/services/provider.py
+++ b/charmcraft/services/provider.py
@@ -17,11 +17,19 @@
 """Service class for creating providers."""
 from __future__ import annotations
 
+import atexit
+
+try:
+    import fcntl
+except ModuleNotFoundError:  # Not available on Windows.
+    fcntl = None  # type: ignore[assignment]
 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 +64,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
diff --git a/tests/conftest.py b/tests/conftest.py
index 1eeecf33d..8b66dd28f 100644
--- a/tests/conftest.py
+++ b/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
diff --git a/tests/integration/services/conftest.py b/tests/integration/services/conftest.py
index 04704da92..96f232f06 100644
--- a/tests/integration/services/conftest.py
+++ b/tests/integration/services/conftest.py
@@ -14,10 +14,7 @@
 #
 # 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
@@ -25,20 +22,10 @@
 
 
 @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)
diff --git a/tests/integration/services/test_package.py b/tests/integration/services/test_package.py
index cfe9fc0b8..7f21f98b5 100644
--- a/tests/integration/services/test_package.py
+++ b/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)
diff --git a/tests/integration/services/test_provider.py b/tests/integration/services/test_provider.py
new file mode 100644
index 000000000..31fbcb0f5
--- /dev/null
+++ b/tests/integration/services/test_provider.py
@@ -0,0 +1,52 @@
+# 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 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_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__).parent,
+        "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()
diff --git a/tests/unit/services/test_provider.py b/tests/unit/services/test_provider.py
index b55f6c48d..a4963725c 100644
--- a/tests/unit/services/test_provider.py
+++ b/tests/unit/services/test_provider.py
@@ -15,12 +15,23 @@
 # For further info, check https://github.com/canonical/charmcraft
 """Unit tests for the provider service."""
 
+try:
+    import fcntl
+except ModuleNotFoundError:  # Windows
+    fcntl = None
+import functools
 import pathlib
+import sys
+from collections.abc import Iterator
+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 +53,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",
     [
@@ -62,6 +84,7 @@ def provider_service(
         bases.BaseName("almalinux", "9"),
     ],
 )
+@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
 def test_get_base_forwards_cache(
     monkeypatch,
     provider_service: services.ProviderService,
@@ -76,3 +99,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),
+    ]

From 7e134dd34bab9bb9b528c18b6bc449c38099019a Mon Sep 17 00:00:00 2001
From: Alex Lowe <alex.lowe@canonical.com>
Date: Thu, 12 Sep 2024 13:57:11 -0400
Subject: [PATCH 2/4] fix: pr comments

---
 .github/workflows/tests.yaml                |  2 --
 tests/integration/services/test_provider.py | 39 +++++++++++++++++----
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
index ac9498234..d8252bcff 100644
--- a/.github/workflows/tests.yaml
+++ b/.github/workflows/tests.yaml
@@ -82,8 +82,6 @@ jobs:
           sudo apt install -y python3-pip python3-setuptools python3-wheel python3-venv libapt-pkg-dev
       - name: Setup LXD
         uses: canonical/setup-lxd@v0.1.1
-        with:
-          channel: latest/stable
         if: ${{ runner.os == 'Linux' }}
       - name: Install skopeo (mac)
         # This is only necessary for Linux until skopeo >= 1.11 is in repos.
diff --git a/tests/integration/services/test_provider.py b/tests/integration/services/test_provider.py
index 31fbcb0f5..ba93b5bdd 100644
--- a/tests/integration/services/test_provider.py
+++ b/tests/integration/services/test_provider.py
@@ -27,25 +27,50 @@
 
 
 @pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
-def test_locks_cache(
+def test_lock_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()
+    cache_path = tmp_path / "cache"
+    cache_path.mkdir()
     provider = service_factory.provider
     provider_kwargs = {
         "build_info": default_build_info,
         "work_dir": pathlib.Path(__file__).parent,
-        "cache_path": tmp_path,
+        "cache_path": cache_path,
+    }
+    assert not (cache_path / "charmcraft.lock").exists()
+
+    with provider.instance(**provider_kwargs):
+        # Test that the cache lock gets created
+        assert (cache_path / "charmcraft.lock").is_file()
+
+    (cache_path / "charmcraft.lock").write_text("Test that file isn't locked.")
+
+
+@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()
+    _maybe_lock_cache(cache_path)
+    assert (cache_path / "charmcraft.lock").exists()
+    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:
-        # 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
+        # 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"])

From 325e7410db9d93c5329e2b1a3e01b2e33143bc96 Mon Sep 17 00:00:00 2001
From: Alex Lowe <alex.lowe@canonical.com>
Date: Fri, 13 Sep 2024 12:45:56 -0400
Subject: [PATCH 3/4] fix: better locking

---
 charmcraft/services/provider.py             | 73 +++++++++++++++++----
 pyproject.toml                              |  3 +
 tests/integration/services/test_provider.py | 37 ++++++++---
 tests/unit/services/test_provider.py        | 17 ++---
 4 files changed, 99 insertions(+), 31 deletions(-)

diff --git a/charmcraft/services/provider.py b/charmcraft/services/provider.py
index 951cca117..443e1fb8b 100644
--- a/charmcraft/services/provider.py
+++ b/charmcraft/services/provider.py
@@ -17,7 +17,11 @@
 """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
 
 try:
     import fcntl
@@ -27,17 +31,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: pathlib.Path | None = None
+        self._lock: io.TextIOBase | None = None
+
     def setup(self) -> None:
         """Set up the provider service for Charmcraft."""
         super().setup()
@@ -64,11 +92,13 @@ 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,
@@ -76,11 +106,31 @@ def get_base(
             **kwargs,  # type: ignore[arg-type]
         )
 
-
-def _maybe_lock_cache(path: pathlib.Path) -> bool:
+    @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 False
+        return None
     cache_lock_path = path / "charmcraft.lock"
 
     emit.trace("Attempting to lock the cache path")
@@ -92,14 +142,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
diff --git a/pyproject.toml b/pyproject.toml
index 128bfd863..f967a85a0 100644
--- a/pyproject.toml
+++ b/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
diff --git a/tests/integration/services/test_provider.py b/tests/integration/services/test_provider.py
index ba93b5bdd..6294e29bc 100644
--- a/tests/integration/services/test_provider.py
+++ b/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()
diff --git a/tests/unit/services/test_provider.py b/tests/unit/services/test_provider.py
index a4963725c..be6bc72de 100644
--- a/tests/unit/services/test_provider.py
+++ b/tests/unit/services/test_provider.py
@@ -151,16 +151,13 @@ def test_get_base_no_cache_if_locked(
 
 
 @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)])
+def test_maybe_lock_cache_locks_single_lock(tmp_path: pathlib.Path) -> None:
+    assert _maybe_lock_cache(tmp_path)
 
 
 @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),
-    ]
+def test_maybe_lock_cache_with_another_lock(tmp_path: pathlib.Path) -> None:
+    # Need to save the open file so it's not closed when we try a second time.
+    first_file_descriptor = _maybe_lock_cache(tmp_path)
+    assert first_file_descriptor
+    assert _maybe_lock_cache(tmp_path) is None

From 12f6665236f79342d016e91acfc785fea7d6d2ea Mon Sep 17 00:00:00 2001
From: Alex Lowe <alex.lowe@canonical.com>
Date: Fri, 13 Sep 2024 18:14:07 -0400
Subject: [PATCH 4/4] uhh some machines don't have flock?

---
 tests/integration/services/test_provider.py | 23 +++++++++++++--------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/integration/services/test_provider.py b/tests/integration/services/test_provider.py
index 6294e29bc..a6b149f99 100644
--- a/tests/integration/services/test_provider.py
+++ b/tests/integration/services/test_provider.py
@@ -16,6 +16,7 @@
 """Integration tests for the provider service."""
 
 import pathlib
+import shutil
 import subprocess
 import sys
 
@@ -37,7 +38,7 @@ 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"]
+    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,
@@ -49,12 +50,14 @@ def test_lock_cache(
     with provider.instance(**provider_kwargs):
         # Test that the cache lock gets created
         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)
+        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.
-    subprocess.run(bash_lock_cmd, check=True)
+    if bash_lock_cmd:
+        subprocess.run(bash_lock_cmd, check=True)
 
 
 @pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows")
@@ -68,13 +71,15 @@ def test_locked_cache_no_cache(
     cache_path.mkdir()
     lock_file = cache_path / "charmcraft.lock"
 
-    bash_lock_cmd = ["bash", "-c", f"flock -n {lock_file} true"]
+    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.
-    subprocess.run(bash_lock_cmd, check=True)
+    if bash_lock_cmd:
+        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)
+    if bash_lock_cmd:
+        with pytest.raises(subprocess.CalledProcessError):
+            subprocess.run(bash_lock_cmd, check=True)
 
     provider = service_factory.provider
     provider_kwargs = {