fix: synchronize with udev via BSD flock after losetup to avoid race condition

imagecraft/pack/image.py

@@ -15,6 +15,7 @@
 """Image handling."""
 
 import contextlib
+import fcntl
 import json
 from collections.abc import Generator, Iterator
 from pathlib import Path
@@ -88,11 +89,17 @@ def attach_loopdev(self) -> Iterator[str]:
                 "--partscan",
                 self.disk_path,
             ).stdout.strip()
-            emit.debug(
-                f"Attached image {self.disk_path} as loop device {self.loop_device}"
-            )
         try:
-            yield self.loop_device
+            # Acquire a shared BSD lock on the loop device.  udev holds an
+            # exclusive lock while it processes the new device; taking a shared
+            # lock here blocks until udev is done and then holds it for the
+            # duration of the context so udev does not interfere with our use.
+            with open(self.loop_device, "rb") as loop_fd:
+                fcntl.flock(loop_fd, fcntl.LOCK_SH)
+                emit.debug(
+                    f"Attached image {self.disk_path} as loop device {self.loop_device}"
+                )
+                yield self.loop_device
         finally:
             self._detach_loopdevs()
 

imagecraft/services/image.py

@@ -16,13 +16,14 @@
 
 import atexit
 import contextlib
+import fcntl
 import json
 import pathlib
 import shutil
 import subprocess
 import time
 from collections.abc import Mapping
-from typing import Any, cast
+from typing import IO, Any, cast
 
 from craft_application import AppMetadata, AppService, ServiceFactory
 from craft_cli import CraftError, emit
@@ -50,6 +51,7 @@ def __init__(
         self._sector_size = gptutil.SECTOR_SIZE_512
         self._images: dict[str, pathlib.Path] | None = None
         self._loop_devices: dict[str, str] = {}
+        self._loop_fds: list[IO[bytes]] = []
         self._atexit_registered = False
 
     def get_images(self) -> Mapping[str, pathlib.Path]:
@@ -156,6 +158,18 @@ def attach_images(self) -> Mapping[str, str]:
                         resolution="Ensure loop devices are available and you have sufficient permissions (sudo).",
                     ) from err
 
+            # 3. Acquire a shared BSD lock on the loop device.  udev holds an
+            # exclusive lock while it processes the device; a shared lock here
+            # blocks until udev is done and then holds it while we use the
+            # device's partitions, preventing udev from interfering.
+            loop_fd = open(attached_device, "rb")  # noqa: SIM115 (held deliberately)
+            try:
+                fcntl.flock(loop_fd, fcntl.LOCK_SH)
+            except Exception:
+                loop_fd.close()
+                raise
+            self._loop_fds.append(loop_fd)
+
             self._loop_devices[name] = attached_device
 
         if not self._atexit_registered:
@@ -169,6 +183,12 @@ def detach_images(self) -> None:
 
         Includes a retry loop for busy devices. Safe to call as an atexit handler.
         """
+        # Release shared flocks so the devices are no longer considered in use.
+        for fd in self._loop_fds:
+            with contextlib.suppress(Exception):
+                fd.close()
+        self._loop_fds.clear()
+
         for name, device in list(self._loop_devices.items()):
             success = False
             start_time = time.monotonic()

tests/unit/pack/test_image.py

@@ -17,6 +17,7 @@
 from typing import Any
 from unittest.mock import call
 
+import fcntl
 import pytest
 from imagecraft.models import Volume
 from imagecraft.pack.image import Image
@@ -48,6 +49,8 @@ def run(cmd: str, *args: Any, **kwargs: Any) -> CompletedProcess[str]:
 class TestImage:
     def test_loopdev(self, mocker, new_dir: Path):
         mock_run = mocker.patch("imagecraft.pack.image.run", side_effect=run)
+        mock_flock = mocker.patch("fcntl.flock")
+        mocker.patch("builtins.open", mocker.mock_open())
 
         volume = Volume.unmarshal(
             {
@@ -78,6 +81,7 @@ def test_loopdev(self, mocker, new_dir: Path):
             call("losetup", "--json"),
             call("losetup", "-d", "/dev/loop99"),
         ]
+        mock_flock.assert_called_once_with(mocker.ANY, fcntl.LOCK_SH)
 
     @pytest.mark.parametrize(
         ("volume_data", "has_data_partition"),
@@ -248,3 +252,4 @@ def test_has_boot_partition(
         )
 
         assert image.has_boot_partition == has_boot_partition
+

tests/unit/services/test_image.py

@@ -12,6 +12,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import fcntl
 import subprocess
 from unittest.mock import MagicMock, patch
 
@@ -104,6 +105,8 @@ def test_attach_images_new(image_service, project_dir, mocker):
     mocker.patch.object(image_service, "_get_all_loop_devices", return_value=[])
 
     mock_run.return_value.stdout = "/dev/loop8\n"
+    mock_flock = mocker.patch("fcntl.flock")
+    mocker.patch("builtins.open", return_value=mocker.MagicMock())
 
     with patch("atexit.register") as mock_atexit:
         devices = image_service.attach_images()
@@ -118,6 +121,8 @@ def test_attach_images_new(image_service, project_dir, mocker):
         )
         mock_atexit.assert_called_once_with(image_service.detach_images)
 
+    mock_flock.assert_called_once_with(mocker.ANY, fcntl.LOCK_SH)
+
 
 def test_attach_images_reuse(image_service, project_dir, mocker):
     image_path = project_dir / ".pc.img.tmp"
@@ -134,12 +139,16 @@ def test_attach_images_reuse(image_service, project_dir, mocker):
     # Mock samefile to return True
     mocker.patch("pathlib.Path.samefile", return_value=True)
     mock_run = mocker.patch("imagecraft.services.image.run")
+    mock_flock = mocker.patch("fcntl.flock")
+    mocker.patch("builtins.open", return_value=mocker.MagicMock())
 
     devices = image_service.attach_images()
 
     assert devices == {"pc": "/dev/loop9"}
     mock_run.assert_not_called()  # Should not call losetup attach
 
+    mock_flock.assert_called_once_with(mocker.ANY, fcntl.LOCK_SH)
+
 
 def test_attach_images_stale_inode(image_service, project_dir, mocker):
     image_path = project_dir / ".pc.img.tmp"
@@ -157,6 +166,8 @@ def test_attach_images_stale_inode(image_service, project_dir, mocker):
     mocker.patch("pathlib.Path.samefile", side_effect=FileNotFoundError)
     mock_run = mocker.patch("imagecraft.services.image.run")
     mock_run.return_value.stdout = "/dev/loop11\n"
+    mocker.patch("fcntl.flock")
+    mocker.patch("builtins.open", return_value=mocker.MagicMock())
 
     devices = image_service.attach_images()
 
@@ -169,6 +180,30 @@ def test_attach_images_stale_inode(image_service, project_dir, mocker):
     )
 
 
+def test_attach_images_flock_released_on_detach(image_service, project_dir, mocker):
+    """attach_images() holds a shared flock; detach_images() closes the fd to release it."""
+    image_service._images = {"pc": project_dir / ".pc.img.tmp"}
+
+    mocker.patch.object(image_service, "_get_all_loop_devices", return_value=[])
+    mock_run = mocker.patch("imagecraft.services.image.run")
+    mock_run.return_value.stdout = "/dev/loop8\n"
+    mocker.patch("fcntl.flock")
+    mock_fd = mocker.MagicMock()
+    mocker.patch("builtins.open", return_value=mock_fd)
+
+    with patch("atexit.register"):
+        image_service.attach_images()
+
+    assert len(image_service._loop_fds) == 1
+
+    # After detach the fd should be closed and the list cleared
+    mock_run.return_value = mocker.MagicMock()  # losetup -d
+    image_service.detach_images()
+
+    mock_fd.close.assert_called_once()
+    assert image_service._loop_fds == []
+
+
 def test_detach_images_success(image_service, mocker):
     image_service._loop_devices = {"pc": "/dev/loop8"}
     mock_run = mocker.patch("imagecraft.services.image.run")