implement atomic_move method, move files only with lock

CONTRIBUTING.md

@@ -165,6 +165,11 @@ Again, the most checks can be performed by `tox` command, though some additional
 
     # Blank line again and package imports
     from ahriman.core.configuration import Configuration
+    # Multiline import example
+    from ahriman.core.database.operations import (
+        AuthOperations,
+        BuildOperations,
+    )
     ```
 
 * One file should define only one class, exception is class satellites in case if file length remains less than 400 lines.

src/ahriman/core/database/sqlite.py

@@ -25,8 +25,16 @@ from typing import Self
 
 from ahriman.core.configuration import Configuration
 from ahriman.core.database.migrations import Migrations
-from ahriman.core.database.operations import AuthOperations, BuildOperations, ChangesOperations, \
+from ahriman.core.database.operations import (
-    DependenciesOperations, EventOperations, LogsOperations, PackageOperations, PatchOperations
+    AuthOperations,
+    BuildOperations,
+    ChangesOperations,
+    DependenciesOperations,
+    EventOperations,
+    LogsOperations,
+    PackageOperations,
+    PatchOperations,
+)
 from ahriman.models.repository_id import RepositoryId
 
 

src/ahriman/core/repository/executor.py

@@ -17,8 +17,6 @@
 # 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 shutil  # shutil.move is used here to ensure cross fs file movement
-
 from collections.abc import Iterable
 from pathlib import Path
 from tempfile import TemporaryDirectory
@@ -27,7 +25,7 @@ from ahriman.core.build_tools.package_archive import PackageArchive
 from ahriman.core.build_tools.task import Task
 from ahriman.core.repository.cleaner import Cleaner
 from ahriman.core.repository.package_info import PackageInfo
-from ahriman.core.utils import safe_filename
+from ahriman.core.utils import atomic_move, safe_filename
 from ahriman.models.changes import Changes
 from ahriman.models.event import EventType
 from ahriman.models.package import Package
@@ -54,7 +52,7 @@ class Executor(PackageInfo, Cleaner):
             return  # suppress type checking, it never can be none actually
 
         if (safe := safe_filename(description.filename)) != description.filename:
-            (self.paths.packages / description.filename).rename(self.paths.packages / safe)
+            atomic_move(self.paths.packages / description.filename, self.paths.packages / safe)
             description.filename = safe
 
     def _package_build(self, package: Package, path: Path, packager: str | None,
@@ -81,7 +79,7 @@ class Executor(PackageInfo, Cleaner):
         package.with_packages(built, self.pacman)
         for src in built:
             dst = self.paths.packages / src.name
-            shutil.move(src, dst)
+            atomic_move(src, dst)
 
         return commit_sha
 
@@ -130,7 +128,7 @@ class Executor(PackageInfo, Cleaner):
 
         for src in files:
             dst = self.paths.archive_for(package_base) / src.name
-            src.rename(dst)  # move package to archive directory
+            atomic_move(src, dst)  # move package to archive directory
             if not (symlink := self.paths.repository / dst.name).exists():
                 symlink.symlink_to(dst.relative_to(symlink.parent, walk_up=True))  # create link to archive
 

src/ahriman/core/utils.py

@@ -19,12 +19,14 @@
 #
 # pylint: disable=too-many-lines
 import datetime
+import fcntl
 import io
 import itertools
 import logging
 import os
 import re
 import selectors
+import shutil
 import subprocess
 
 from collections.abc import Callable, Generator, Iterable, Mapping
@@ -39,6 +41,7 @@ from ahriman.models.repository_paths import RepositoryPaths
 
 
 __all__ = [
+    "atomic_move",
     "check_output",
     "check_user",
     "dataclass_view",
@@ -65,6 +68,34 @@ __all__ = [
 T = TypeVar("T")
 
 
+def atomic_move(src: Path, dst: Path) -> None:
+    """
+    move file from ``source`` location to ``destination``. This method uses lock and :func:`shutil.move` to ensure that
+    file will be copied (if not rename) atomically. This method blocks execution until lock is available
+
+    Args:
+        src(Path): path to the source file
+        dst(Path): path to the destination
+
+    Examples:
+        This method is a drop-in replacement for :func:`shutil.move` (except it doesn't allow to override copy method)
+        which first locking destination file. To use it simply call method with arguments::
+
+            >>> atomic_move(src, dst)
+    """
+    lock_path = dst.with_name(f".{dst.name}")
+    try:
+        with lock_path.open("ab") as lock_file:
+            fd = lock_file.fileno()
+            try:
+                fcntl.flock(fd, fcntl.LOCK_EX)  # lock file and wait lock is until available
+                shutil.move(src, dst)
+            finally:
+                fcntl.flock(fd, fcntl.LOCK_UN)  # unlock file first
+    finally:
+        lock_path.unlink(missing_ok=True)  # remove lock file at the end
+
+
 # pylint: disable=too-many-locals
 def check_output(*args: str, exception: Exception | Callable[[int, list[str], str, str], Exception] | None = None,
                  cwd: Path | None = None, input_data: str | None = None,

src/ahriman/web/middlewares/exception_handler.py

@@ -21,8 +21,18 @@ import aiohttp_jinja2
 import logging
 
 from aiohttp.typedefs import Middleware
-from aiohttp.web import HTTPClientError, HTTPException, HTTPMethodNotAllowed, HTTPNoContent, HTTPServerError, \
+from aiohttp.web import (
-    HTTPUnauthorized, Request, StreamResponse, json_response, middleware
+    HTTPClientError,
+    HTTPException,
+    HTTPMethodNotAllowed,
+    HTTPNoContent,
+    HTTPServerError,
+    HTTPUnauthorized,
+    Request,
+    StreamResponse,
+    json_response,
+    middleware,
+)
 
 from ahriman.web.middlewares import HandlerType
 

src/ahriman/web/views/v1/packages/package.py

@@ -25,8 +25,12 @@ from ahriman.models.build_status import BuildStatusEnum
 from ahriman.models.package import Package
 from ahriman.models.user_access import UserAccess
 from ahriman.web.apispec.decorators import apidocs
-from ahriman.web.schemas import PackageNameSchema, PackageStatusSchema, PackageStatusSimplifiedSchema, \
+from ahriman.web.schemas import (
-    RepositoryIdSchema
+    PackageNameSchema,
+    PackageStatusSchema,
+    PackageStatusSimplifiedSchema,
+    RepositoryIdSchema,
+)
 from ahriman.web.views.base import BaseView
 from ahriman.web.views.status_view_guard import StatusViewGuard
 

src/ahriman/web/views/v1/service/upload.py

@@ -26,6 +26,7 @@ from tempfile import NamedTemporaryFile
 from typing import ClassVar
 
 from ahriman.core.configuration import Configuration
+from ahriman.core.utils import atomic_move
 from ahriman.models.repository_paths import RepositoryPaths
 from ahriman.models.user_access import UserAccess
 from ahriman.web.apispec.decorators import apidocs
@@ -152,10 +153,8 @@ class UploadView(BaseView):
 
             files.append(await self.save_file(part, target, max_body_size=max_body_size))
 
-        # and now we can rename files, which is relatively fast operation
-        # it is probably good way to call lock here, however
         for filename, current_location in files:
             target_location = current_location.parent / filename
-            current_location.rename(target_location)
+            atomic_move(current_location, target_location)
 
         raise HTTPCreated

tests/ahriman/core/repository/test_executor.py

@@ -20,10 +20,10 @@ def test_archive_rename(executor: Executor, package_ahriman: Package, mocker: Mo
     path = "gconf-3.2.6+11+g07808097-10-x86_64.pkg.tar.zst"
     safe_path = "gconf-3.2.6-11-g07808097-10-x86_64.pkg.tar.zst"
     package_ahriman.packages[package_ahriman.base].filename = path
-    rename_mock = mocker.patch("pathlib.Path.rename")
+    rename_mock = mocker.patch("ahriman.core.repository.executor.atomic_move")
 
     executor._archive_rename(package_ahriman.packages[package_ahriman.base], package_ahriman.base)
-    rename_mock.assert_called_once_with(executor.paths.packages / safe_path)
+    rename_mock.assert_called_once_with(executor.paths.packages / path, executor.paths.packages / safe_path)
     assert package_ahriman.packages[package_ahriman.base].filename == safe_path
 
 
@@ -32,7 +32,7 @@ def test_archive_rename_empty_filename(executor: Executor, package_ahriman: Pack
     must skip renaming if filename is not set
     """
     package_ahriman.packages[package_ahriman.base].filename = None
-    rename_mock = mocker.patch("pathlib.Path.rename")
+    rename_mock = mocker.patch("ahriman.core.repository.executor.atomic_move")
 
     executor._archive_rename(package_ahriman.packages[package_ahriman.base], package_ahriman.base)
     rename_mock.assert_not_called()
@@ -46,13 +46,13 @@ def test_package_build(executor: Executor, package_ahriman: Package, mocker: Moc
     status_client_mock = mocker.patch("ahriman.core.status.Client.set_building")
     init_mock = mocker.patch("ahriman.core.build_tools.task.Task.init", return_value="sha")
     with_packages_mock = mocker.patch("ahriman.models.package.Package.with_packages")
-    move_mock = mocker.patch("shutil.move")
+    rename_mock = mocker.patch("ahriman.core.repository.executor.atomic_move")
 
     assert executor._package_build(package_ahriman, Path("local"), "packager", None) == "sha"
     status_client_mock.assert_called_once_with(package_ahriman.base)
     init_mock.assert_called_once_with(pytest.helpers.anyvar(int), pytest.helpers.anyvar(int), None)
     with_packages_mock.assert_called_once_with([Path(package_ahriman.base)], executor.pacman)
-    move_mock.assert_called_once_with(Path(package_ahriman.base), executor.paths.packages / package_ahriman.base)
+    rename_mock.assert_called_once_with(Path(package_ahriman.base), executor.paths.packages / package_ahriman.base)
 
 
 def test_package_remove(executor: Executor, package_ahriman: Package, mocker: MockerFixture) -> None:
@@ -94,7 +94,7 @@ def test_package_update(executor: Executor, package_ahriman: Package, user: User
     """
     must update built package in repository
     """
-    rename_mock = mocker.patch("pathlib.Path.rename")
+    rename_mock = mocker.patch("ahriman.core.repository.executor.atomic_move")
     symlink_mock = mocker.patch("pathlib.Path.symlink_to")
     repo_add_mock = mocker.patch("ahriman.core.alpm.repo.Repo.add")
     sign_package_mock = mocker.patch("ahriman.core.sign.gpg.GPG.process_sign_package", side_effect=lambda fn, _: [fn])
@@ -102,7 +102,8 @@ def test_package_update(executor: Executor, package_ahriman: Package, user: User
 
     executor._package_update(filepath, package_ahriman.base, user.key)
     # must move files (once)
-    rename_mock.assert_called_once_with(executor.paths.archive_for(package_ahriman.base) / filepath)
+    rename_mock.assert_called_once_with(
+        executor.paths.packages / filepath, executor.paths.archive_for(package_ahriman.base) / filepath)
     # must sign package
     sign_package_mock.assert_called_once_with(executor.paths.packages / filepath, user.key)
     # symlink to the archive
@@ -122,7 +123,7 @@ def test_package_update_empty_filename(executor: Executor, package_ahriman: Pack
     """
     must skip update for package which does not have path
     """
-    rename_mock = mocker.patch("pathlib.Path.rename")
+    rename_mock = mocker.patch("ahriman.core.repository.executor.atomic_move")
     executor._package_update(None, package_ahriman.base, None)
     rename_mock.assert_not_called()
 
@@ -155,7 +156,7 @@ def test_process_build_bump_pkgrel(executor: Executor, package_ahriman: Package,
     """
     mocker.patch("ahriman.core.repository.executor.Executor.packages", return_value=[package_ahriman])
     mocker.patch("ahriman.core.build_tools.task.Task.build", return_value=[Path(package_ahriman.base)])
-    mocker.patch("shutil.move")
+    mocker.patch("ahriman.core.repository.executor.atomic_move")
     init_mock = mocker.patch("ahriman.core.build_tools.task.Task.init")
 
     executor.process_build([package_ahriman], Packagers("packager"), bump_pkgrel=True)
@@ -172,7 +173,7 @@ def test_process_build_failure(executor: Executor, package_ahriman: Package, moc
     mocker.patch("ahriman.core.repository.executor.Executor.packages_built")
     mocker.patch("ahriman.core.build_tools.task.Task.build", return_value=[Path(package_ahriman.base)])
     mocker.patch("ahriman.core.build_tools.task.Task.init")
-    mocker.patch("shutil.move", side_effect=Exception)
+    mocker.patch("ahriman.core.repository.executor.atomic_move", side_effect=Exception)
     status_client_mock = mocker.patch("ahriman.core.status.Client.set_failed")
 
     executor.process_build([package_ahriman])

tests/ahriman/core/test_utils.py

@@ -1,4 +1,5 @@
 import datetime
+import fcntl
 import logging
 import os
 import pytest
@@ -9,15 +10,82 @@ from typing import Any
 from unittest.mock import call as MockCall
 
 from ahriman.core.exceptions import BuildError, CalledProcessError, OptionError, UnsafeRunError
-from ahriman.core.utils import check_output, check_user, dataclass_view, enum_values, extract_user, filter_json, \
+from ahriman.core.utils import (
-    full_version, minmax, package_like, parse_version, partition, pretty_datetime, pretty_interval, pretty_size, \
+    atomic_move,
-    safe_filename, srcinfo_property, srcinfo_property_list, trim_package, utcnow, walk
+    check_output,
+    check_user,
+    dataclass_view,
+    enum_values,
+    extract_user,
+    filter_json,
+    full_version,
+    minmax,
+    package_like,
+    parse_version,
+    partition,
+    pretty_datetime,
+    pretty_interval,
+    pretty_size,
+    safe_filename,
+    srcinfo_property,
+    srcinfo_property_list,
+    trim_package,
+    utcnow,
+    walk,
+)
 from ahriman.models.package import Package
 from ahriman.models.package_source import PackageSource
 from ahriman.models.repository_id import RepositoryId
 from ahriman.models.repository_paths import RepositoryPaths
 
 
+def test_atomic_move(mocker: MockerFixture) -> None:
+    """
+    must move file with locking
+    """
+    lock_mock = mocker.patch("fcntl.flock")
+    open_mock = mocker.patch("pathlib.Path.open", autospec=True)
+    move_mock = mocker.patch("shutil.move")
+    unlink_mock = mocker.patch("pathlib.Path.unlink")
+
+    atomic_move(Path("source"), Path("destination"))
+    open_mock.assert_called_once_with(Path(".destination"), "ab")
+    lock_mock.assert_has_calls([
+        MockCall(pytest.helpers.anyvar(int), fcntl.LOCK_EX),
+        MockCall(pytest.helpers.anyvar(int), fcntl.LOCK_UN),
+    ])
+    move_mock.assert_called_once_with(Path("source"), Path("destination"))
+    unlink_mock.assert_called_once_with(missing_ok=True)
+
+
+def test_atomic_move_remove_lock(mocker: MockerFixture) -> None:
+    """
+    must remove lock file in case of exception
+    """
+    mocker.patch("pathlib.Path.open", side_effect=Exception)
+    unlink_mock = mocker.patch("pathlib.Path.unlink")
+
+    with pytest.raises(Exception):
+        atomic_move(Path("source"), Path("destination"))
+    unlink_mock.assert_called_once_with(missing_ok=True)
+
+
+def test_atomic_move_unlock(mocker: MockerFixture) -> None:
+    """
+    must unlock file in case of exception
+    """
+    mocker.patch("pathlib.Path.open")
+    mocker.patch("shutil.move", side_effect=Exception)
+    lock_mock = mocker.patch("fcntl.flock")
+
+    with pytest.raises(Exception):
+        atomic_move(Path("source"), Path("destination"))
+    lock_mock.assert_has_calls([
+        MockCall(pytest.helpers.anyvar(int), fcntl.LOCK_EX),
+        MockCall(pytest.helpers.anyvar(int), fcntl.LOCK_UN),
+    ])
+
+
 def test_check_output(mocker: MockerFixture) -> None:
     """
     must run command and log result

tests/ahriman/web/views/v1/service/test_view_v1_service_upload.py

@@ -109,7 +109,7 @@ async def test_post(client: TestClient, repository_paths: RepositoryPaths, mocke
     local = Path("local")
     save_mock = pytest.helpers.patch_view(client.app, "save_file",
                                           AsyncMock(return_value=("filename", local / ".filename")))
-    rename_mock = mocker.patch("pathlib.Path.rename")
+    rename_mock = mocker.patch("ahriman.web.views.v1.service.upload.atomic_move")
     # no content validation here because it has invalid schema
 
     data = FormData()
@@ -118,7 +118,7 @@ async def test_post(client: TestClient, repository_paths: RepositoryPaths, mocke
     response = await client.post("/api/v1/service/upload", data=data)
     assert response.ok
     save_mock.assert_called_once_with(pytest.helpers.anyvar(int), repository_paths.packages, max_body_size=None)
-    rename_mock.assert_called_once_with(local / "filename")
+    rename_mock.assert_called_once_with(local / ".filename", local / "filename")
 
 
 async def test_post_with_sig(client: TestClient, repository_paths: RepositoryPaths, mocker: MockerFixture) -> None:
@@ -131,7 +131,7 @@ async def test_post_with_sig(client: TestClient, repository_paths: RepositoryPat
                                               ("filename", local / ".filename"),
                                               ("filename.sig", local / ".filename.sig"),
                                           ]))
-    rename_mock = mocker.patch("pathlib.Path.rename")
+    rename_mock = mocker.patch("ahriman.web.views.v1.service.upload.atomic_move")
     # no content validation here because it has invalid schema
 
     data = FormData()
@@ -145,8 +145,8 @@ async def test_post_with_sig(client: TestClient, repository_paths: RepositoryPat
         MockCall(pytest.helpers.anyvar(int), repository_paths.packages, max_body_size=None),
     ])
     rename_mock.assert_has_calls([
-        MockCall(local / "filename"),
+        MockCall(local / ".filename", local / "filename"),
-        MockCall(local / "filename.sig"),
+        MockCall(local / ".filename.sig", local / "filename.sig"),
     ])