From c5d849d6a6836cebeef90b325b3ec45b659dd74b Mon Sep 17 00:00:00 2001 From: Evgenii Alekseev Date: Wed, 30 Jul 2025 14:45:29 +0300 Subject: [PATCH] implement atomic_move method, move files only with lock --- CONTRIBUTING.md | 5 ++ src/ahriman/core/database/sqlite.py | 12 ++- src/ahriman/core/repository/executor.py | 10 +-- src/ahriman/core/utils.py | 31 ++++++++ .../web/middlewares/exception_handler.py | 14 +++- src/ahriman/web/views/v1/packages/package.py | 8 +- src/ahriman/web/views/v1/service/upload.py | 5 +- .../ahriman/core/repository/test_executor.py | 21 +++--- tests/ahriman/core/test_utils.py | 74 ++++++++++++++++++- .../v1/service/test_view_v1_service_upload.py | 10 +-- 10 files changed, 157 insertions(+), 33 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1ed8f87a..6bf9118d 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/src/ahriman/core/database/sqlite.py b/src/ahriman/core/database/sqlite.py index 46759679..a00adb39 100644 --- a/src/ahriman/core/database/sqlite.py +++ b/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, \ - DependenciesOperations, EventOperations, LogsOperations, PackageOperations, PatchOperations +from ahriman.core.database.operations import ( + AuthOperations, + BuildOperations, + ChangesOperations, + DependenciesOperations, + EventOperations, + LogsOperations, + PackageOperations, + PatchOperations, +) from ahriman.models.repository_id import RepositoryId diff --git a/src/ahriman/core/repository/executor.py b/src/ahriman/core/repository/executor.py index dc2197d4..0ac135f4 100644 --- a/src/ahriman/core/repository/executor.py +++ b/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 . # -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 diff --git a/src/ahriman/core/utils.py b/src/ahriman/core/utils.py index 632b8115..88c655b4 100644 --- a/src/ahriman/core/utils.py +++ b/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, diff --git a/src/ahriman/web/middlewares/exception_handler.py b/src/ahriman/web/middlewares/exception_handler.py index ebc73e0d..cf19e994 100644 --- a/src/ahriman/web/middlewares/exception_handler.py +++ b/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, \ - HTTPUnauthorized, Request, StreamResponse, json_response, middleware +from aiohttp.web import ( + HTTPClientError, + HTTPException, + HTTPMethodNotAllowed, + HTTPNoContent, + HTTPServerError, + HTTPUnauthorized, + Request, + StreamResponse, + json_response, + middleware, +) from ahriman.web.middlewares import HandlerType diff --git a/src/ahriman/web/views/v1/packages/package.py b/src/ahriman/web/views/v1/packages/package.py index 0a1239a0..37591e84 100644 --- a/src/ahriman/web/views/v1/packages/package.py +++ b/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, \ - RepositoryIdSchema +from ahriman.web.schemas import ( + PackageNameSchema, + PackageStatusSchema, + PackageStatusSimplifiedSchema, + RepositoryIdSchema, +) from ahriman.web.views.base import BaseView from ahriman.web.views.status_view_guard import StatusViewGuard diff --git a/src/ahriman/web/views/v1/service/upload.py b/src/ahriman/web/views/v1/service/upload.py index 7d15b60b..eb8d8553 100644 --- a/src/ahriman/web/views/v1/service/upload.py +++ b/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 diff --git a/tests/ahriman/core/repository/test_executor.py b/tests/ahriman/core/repository/test_executor.py index 9b015ba2..93d339f6 100644 --- a/tests/ahriman/core/repository/test_executor.py +++ b/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]) diff --git a/tests/ahriman/core/test_utils.py b/tests/ahriman/core/test_utils.py index 100670a9..08871dee 100644 --- a/tests/ahriman/core/test_utils.py +++ b/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, \ - 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.core.utils import ( + atomic_move, + 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 diff --git a/tests/ahriman/web/views/v1/service/test_view_v1_service_upload.py b/tests/ahriman/web/views/v1/service/test_view_v1_service_upload.py index 7ab7f41a..c8bef0f6 100644 --- a/tests/ahriman/web/views/v1/service/test_view_v1_service_upload.py +++ b/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.sig"), + MockCall(local / ".filename", local / "filename"), + MockCall(local / ".filename.sig", local / "filename.sig"), ])