From 53cc81b11ff194a4f26fb875e59b0bc7fc7178a7 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 15:21:42 +0300 Subject: [PATCH 01/10] Add test for parallel refresh This likely fails on all platforms right now, but the Windows behaviour cannot be fixed without actual locking. Signed-off-by: Jussi Kukkonen --- tests/refresh_script.py | 15 +++++++++++++++ tests/test_updater_ng.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 tests/refresh_script.py diff --git a/tests/refresh_script.py b/tests/refresh_script.py new file mode 100644 index 0000000000..07ac17ef3b --- /dev/null +++ b/tests/refresh_script.py @@ -0,0 +1,15 @@ +import sys + +from tuf.ngclient import Updater + +print(f"Creating and refreshing a client {sys.argv[1]} times:") +print(f" metadata dir: {sys.argv[2]}") +print(f" metadata url: {sys.argv[3]}") + + +for i in range(int(sys.argv[1])): + try: + u = Updater(metadata_dir=sys.argv[2], metadata_base_url=sys.argv[3]) + u.refresh() + except OSError as e: + sys.exit(f"Failed on iteration {i}: {e}") diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py index 50ef5ee3be..aa6de64f91 100644 --- a/tests/test_updater_ng.py +++ b/tests/test_updater_ng.py @@ -10,6 +10,7 @@ import shutil import sys import tempfile +import subprocess import unittest from collections.abc import Iterable from typing import TYPE_CHECKING, Callable, ClassVar @@ -354,6 +355,44 @@ def test_user_agent(self) -> None: self.assertEqual(ua[:23], "MyApp/1.2.3 python-tuf/") +class TestParallelUpdater(TestUpdater): + def test_parallel_updaters(self) -> None: + # Refresh two updaters in parallel many times, using the same local metadata cache. + # This should reveal race conditions. + + iterations = 100 + + # The project root is the parent of the tests directory + project_root = os.path.dirname(utils.TESTS_DIR) + + command = [ + sys.executable, + "-m", + "tests.refresh_script", + str(iterations), + self.client_directory, + self.metadata_url, + ] + + p1 = subprocess.Popen( + command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=project_root + ) + p2 = subprocess.Popen( + command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=project_root + ) + + stdout1, stderr1 = p1.communicate() + stdout2, stderr2 = p2.communicate() + + if p1.returncode != 0 or p2.returncode != 0: + self.fail( + "Parallel refresh failed" + f"\nprocess 1 stdout: \n{stdout1.decode()}" + f"\nprocess 1 stderr: \n{stderr1.decode()}" + f"\nprocess 2 stdout: \n{stdout2.decode()}" + f"\nprocess 2 stderr: \n{stderr2.decode()}" + ) + if __name__ == "__main__": utils.configure_test_logging(sys.argv) unittest.main() From eeb59f8484c79fc8408ccb010fe7c13f067fe60d Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 16:26:02 +0300 Subject: [PATCH 02/10] ngclient: Advisory locking, first draft Signed-off-by: Jussi Kukkonen --- tests/test_updater_delegation_graphs.py | 2 +- tests/test_updater_ng.py | 4 +- tests/test_updater_top_level_update.py | 9 ++-- tests/utils.py | 2 +- tuf/ngclient/updater.py | 62 ++++++++++++++++++++----- 5 files changed, 61 insertions(+), 18 deletions(-) diff --git a/tests/test_updater_delegation_graphs.py b/tests/test_updater_delegation_graphs.py index 770a1b3d71..f917bfed94 100644 --- a/tests/test_updater_delegation_graphs.py +++ b/tests/test_updater_delegation_graphs.py @@ -136,7 +136,7 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None: """Assert that local metadata files match 'roles'""" expected_files = [f"{role}.json" for role in roles] found_files = [ - e.name for e in os.scandir(self.metadata_dir) if e.is_file() + e.name for e in os.scandir(self.metadata_dir) if e.is_file() and e.name != ".lock" ] self.assertListEqual(sorted(found_files), sorted(expected_files)) diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py index aa6de64f91..151dc86dfc 100644 --- a/tests/test_updater_ng.py +++ b/tests/test_updater_ng.py @@ -8,9 +8,9 @@ import logging import os import shutil +import subprocess import sys import tempfile -import subprocess import unittest from collections.abc import Iterable from typing import TYPE_CHECKING, Callable, ClassVar @@ -158,7 +158,7 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None: """Assert that local metadata files match 'roles'""" expected_files = [f"{role}.json" for role in roles] found_files = [ - e.name for e in os.scandir(self.client_directory) if e.is_file() + e.name for e in os.scandir(self.client_directory) if e.is_file() and e.name != ".lock" ] self.assertListEqual(sorted(found_files), sorted(expected_files)) diff --git a/tests/test_updater_top_level_update.py b/tests/test_updater_top_level_update.py index 76c74d4b57..161858f4ae 100644 --- a/tests/test_updater_top_level_update.py +++ b/tests/test_updater_top_level_update.py @@ -94,7 +94,7 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None: """Assert that local metadata files match 'roles'""" expected_files = [f"{role}.json" for role in roles] found_files = [ - e.name for e in os.scandir(self.metadata_dir) if e.is_file() + e.name for e in os.scandir(self.metadata_dir) if e.is_file() and e.name != ".lock" ] self.assertListEqual(sorted(found_files), sorted(expected_files)) @@ -644,14 +644,16 @@ def test_not_loading_targets_twice(self, wrapped_open: MagicMock) -> None: wrapped_open.reset_mock() # First time looking for "somepath", only 'role1' must be loaded + # (and ".lock" for metadata locking) updater.get_targetinfo("somepath") - wrapped_open.assert_called_once_with( + self.assertEqual(wrapped_open.call_count, 2) + wrapped_open.assert_called_with( os.path.join(self.metadata_dir, "role1.json"), "rb" ) wrapped_open.reset_mock() # Second call to get_targetinfo, all metadata is already loaded updater.get_targetinfo("somepath") - wrapped_open.assert_not_called() + self.assertEqual(wrapped_open.call_count, 1) def test_snapshot_rollback_with_local_snapshot_hash_mismatch(self) -> None: # Test triggering snapshot rollback check on a newly downloaded snapshot @@ -709,6 +711,7 @@ def test_load_metadata_from_cache(self, wrapped_open: MagicMock) -> None: root_dir = os.path.join(self.metadata_dir, "root_history") wrapped_open.assert_has_calls( [ + call(os.path.join(self.metadata_dir, ".lock"), "wb"), call(os.path.join(root_dir, "2.root.json"), "rb"), call(os.path.join(self.metadata_dir, "timestamp.json"), "rb"), call(os.path.join(self.metadata_dir, "snapshot.json"), "rb"), diff --git a/tests/utils.py b/tests/utils.py index bbfb07dbaa..727abadb3e 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -161,7 +161,7 @@ def cleanup_metadata_dir(path: str) -> None: for entry in it: if entry.name == "root_history": cleanup_metadata_dir(entry.path) - elif entry.name.endswith(".json"): + elif entry.name.endswith(".json") or entry.name == ".lock": os.remove(entry.path) else: raise ValueError(f"Unexpected local metadata file {entry.path}") diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index a98e799ce4..a347732190 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -59,7 +59,7 @@ import shutil import tempfile from pathlib import Path -from typing import TYPE_CHECKING, cast +from typing import IO, TYPE_CHECKING, cast from urllib import parse from tuf.api import exceptions @@ -69,10 +69,30 @@ from tuf.ngclient.urllib3_fetcher import Urllib3Fetcher if TYPE_CHECKING: + from collections.abc import Iterator + from tuf.ngclient.fetcher import FetcherInterface logger = logging.getLogger(__name__) +try: + # advisory file locking for posix + import fcntl + def _lock_file(f: IO) -> None: + if f.writable(): + fcntl.lockf(f, fcntl.LOCK_EX) + +except ModuleNotFoundError: + # Windows file locking + import msvcrt + + def _lock_file(f: IO) -> None: + # On Windows we lock bytes, not the file + f.write(b"\0") + f.flush() + f.seek(0) + msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) + class Updater: """Creates a new ``Updater`` instance and loads trusted root metadata. @@ -139,8 +159,23 @@ def __init__( self._trusted_set = TrustedMetadataSet( bootstrap, self.config.envelope_type ) - self._persist_root(self._trusted_set.root.version, bootstrap) - self._update_root_symlink() + with self._lock_metadata(): + self._persist_root(self._trusted_set.root.version, bootstrap) + self._update_root_symlink() + + + @contextlib.contextmanager + def _lock_metadata(self) -> Iterator[None]: + """Context manager for locking the metadata directory.""" + # Ensure the whole metadata directory structure exists + rootdir = Path(self._dir, "root_history") + rootdir.mkdir(exist_ok=True, parents=True) + + with open(os.path.join(self._dir, ".lock"), "wb") as f: + logger.debug("Getting metadata lock...") + _lock_file(f) + yield + logger.debug("Releasing metadata lock") def refresh(self) -> None: """Refresh top-level metadata. @@ -166,10 +201,11 @@ def refresh(self) -> None: DownloadError: Download of a metadata file failed in some way """ - self._load_root() - self._load_timestamp() - self._load_snapshot() - self._load_targets(Targets.type, Root.type) + with self._lock_metadata(): + self._load_root() + self._load_timestamp() + self._load_snapshot() + self._load_targets(Targets.type, Root.type) def _generate_target_file_path(self, targetinfo: TargetFile) -> str: if self.target_dir is None: @@ -205,9 +241,14 @@ def get_targetinfo(self, target_path: str) -> TargetFile | None: ``TargetFile`` instance or ``None``. """ - if Targets.type not in self._trusted_set: - self.refresh() - return self._preorder_depth_first_walk(target_path) + with self._lock_metadata(): + if Targets.type not in self._trusted_set: + # refresh + self._load_root() + self._load_timestamp() + self._load_snapshot() + self._load_targets(Targets.type, Root.type) + return self._preorder_depth_first_walk(target_path) def find_cached_target( self, @@ -335,7 +376,6 @@ def _persist_root(self, version: int, data: bytes) -> None: "root_history/1.root.json"). """ rootdir = Path(self._dir, "root_history") - rootdir.mkdir(exist_ok=True, parents=True) self._persist_file(str(rootdir / f"{version}.root.json"), data) def _persist_file(self, filename: str, data: bytes) -> None: From 6d666968df10064a245fe699ccb3da89e02d9359 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 18:42:50 +0300 Subject: [PATCH 03/10] tests: Expand parallel refresh test Use get_targetinfo() so that the delegated role loading is tested as well Signed-off-by: Jussi Kukkonen --- tests/refresh_script.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/refresh_script.py b/tests/refresh_script.py index 07ac17ef3b..ea87d10fd6 100644 --- a/tests/refresh_script.py +++ b/tests/refresh_script.py @@ -2,7 +2,7 @@ from tuf.ngclient import Updater -print(f"Creating and refreshing a client {sys.argv[1]} times:") +print(f"Fetching metadata {sys.argv[1]} times:") print(f" metadata dir: {sys.argv[2]}") print(f" metadata url: {sys.argv[3]}") @@ -10,6 +10,7 @@ for i in range(int(sys.argv[1])): try: u = Updater(metadata_dir=sys.argv[2], metadata_base_url=sys.argv[3]) - u.refresh() + # file3.txt is delegated so we end up exercising all metadata load paths + u.get_targetinfo("file3.txt") except OSError as e: sys.exit(f"Failed on iteration {i}: {e}") From 7a8edd98303503dbf7ea2c917430f6fcff60a6be Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 18:48:55 +0300 Subject: [PATCH 04/10] ngclient: Advisory locking for artifacts This should prevent issues with multiple processes trying to write at same time. Signed-off-by: Jussi Kukkonen --- tuf/ngclient/updater.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index a347732190..08cab467ca 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -337,6 +337,7 @@ def download_target( target_file.seek(0) with open(filepath, "wb") as destination_file: + _lock_file(destination_file) shutil.copyfileobj(target_file, destination_file) logger.debug("Downloaded target %s", targetinfo.path) From b63cf71662b8260a4e29391b8fe014fc9422ab60 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 19:07:12 +0300 Subject: [PATCH 05/10] lint fixes Signed-off-by: Jussi Kukkonen --- tests/test_updater_delegation_graphs.py | 4 +++- tests/test_updater_ng.py | 15 ++++++++++++--- tests/test_updater_top_level_update.py | 4 +++- tuf/ngclient/updater.py | 4 ++-- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/tests/test_updater_delegation_graphs.py b/tests/test_updater_delegation_graphs.py index f917bfed94..f6a6686283 100644 --- a/tests/test_updater_delegation_graphs.py +++ b/tests/test_updater_delegation_graphs.py @@ -136,7 +136,9 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None: """Assert that local metadata files match 'roles'""" expected_files = [f"{role}.json" for role in roles] found_files = [ - e.name for e in os.scandir(self.metadata_dir) if e.is_file() and e.name != ".lock" + e.name + for e in os.scandir(self.metadata_dir) + if e.is_file() and e.name != ".lock" ] self.assertListEqual(sorted(found_files), sorted(expected_files)) diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py index 151dc86dfc..554961c053 100644 --- a/tests/test_updater_ng.py +++ b/tests/test_updater_ng.py @@ -158,7 +158,9 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None: """Assert that local metadata files match 'roles'""" expected_files = [f"{role}.json" for role in roles] found_files = [ - e.name for e in os.scandir(self.client_directory) if e.is_file() and e.name != ".lock" + e.name + for e in os.scandir(self.client_directory) + if e.is_file() and e.name != ".lock" ] self.assertListEqual(sorted(found_files), sorted(expected_files)) @@ -375,10 +377,16 @@ def test_parallel_updaters(self) -> None: ] p1 = subprocess.Popen( - command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=project_root + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + cwd=project_root, ) p2 = subprocess.Popen( - command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=project_root + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + cwd=project_root, ) stdout1, stderr1 = p1.communicate() @@ -393,6 +401,7 @@ def test_parallel_updaters(self) -> None: f"\nprocess 2 stderr: \n{stderr2.decode()}" ) + if __name__ == "__main__": utils.configure_test_logging(sys.argv) unittest.main() diff --git a/tests/test_updater_top_level_update.py b/tests/test_updater_top_level_update.py index 161858f4ae..883e59056c 100644 --- a/tests/test_updater_top_level_update.py +++ b/tests/test_updater_top_level_update.py @@ -94,7 +94,9 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None: """Assert that local metadata files match 'roles'""" expected_files = [f"{role}.json" for role in roles] found_files = [ - e.name for e in os.scandir(self.metadata_dir) if e.is_file() and e.name != ".lock" + e.name + for e in os.scandir(self.metadata_dir) + if e.is_file() and e.name != ".lock" ] self.assertListEqual(sorted(found_files), sorted(expected_files)) diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index 08cab467ca..5bb75fef2a 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -78,6 +78,7 @@ try: # advisory file locking for posix import fcntl + def _lock_file(f: IO) -> None: if f.writable(): fcntl.lockf(f, fcntl.LOCK_EX) @@ -87,7 +88,7 @@ def _lock_file(f: IO) -> None: import msvcrt def _lock_file(f: IO) -> None: - # On Windows we lock bytes, not the file + # On Windows we lock a byte range and file must not be empty f.write(b"\0") f.flush() f.seek(0) @@ -163,7 +164,6 @@ def __init__( self._persist_root(self._trusted_set.root.version, bootstrap) self._update_root_symlink() - @contextlib.contextmanager def _lock_metadata(self) -> Iterator[None]: """Context manager for locking the metadata directory.""" From ba3adef2b4733eefaf972cf4794667882b3ad4a1 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 19:32:32 +0300 Subject: [PATCH 06/10] ngclient: Move bootstrap root loading inside lock Otherwise another process might delete the file underneath us Signed-off-by: Jussi Kukkonen --- tuf/ngclient/updater.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index 5bb75fef2a..cfac656926 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -92,6 +92,7 @@ def _lock_file(f: IO) -> None: f.write(b"\0") f.flush() f.seek(0) + msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) @@ -152,15 +153,15 @@ def __init__( f"got '{self.config.envelope_type}'" ) - if not bootstrap: - # if no root was provided, use the cached non-versioned root.json - bootstrap = self._load_local_metadata(Root.type) - - # Load the initial root, make sure it's cached - self._trusted_set = TrustedMetadataSet( - bootstrap, self.config.envelope_type - ) with self._lock_metadata(): + if not bootstrap: + # if no root was provided, use the cached non-versioned root + bootstrap = self._load_local_metadata(Root.type) + + # Load the initial root, make sure it's cached + self._trusted_set = TrustedMetadataSet( + bootstrap, self.config.envelope_type + ) self._persist_root(self._trusted_set.root.version, bootstrap) self._update_root_symlink() From cbe34d956a2333ca814f63c5e261d8c77351f6f0 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 19:46:27 +0300 Subject: [PATCH 07/10] tests: Fix check to be compatible with .lock Signed-off-by: Jussi Kukkonen --- tests/test_updater_top_level_update.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_updater_top_level_update.py b/tests/test_updater_top_level_update.py index 883e59056c..c588661220 100644 --- a/tests/test_updater_top_level_update.py +++ b/tests/test_updater_top_level_update.py @@ -135,8 +135,7 @@ def test_cached_root_missing_without_bootstrap(self) -> None: self._run_refresh(skip_bootstrap=True) # Metadata dir is empty - with self.assertRaises(FileNotFoundError): - os.listdir(self.metadata_dir) + self._assert_files_exist([]) def test_trusted_root_expired(self) -> None: # Create an expired root version From ba0842ff72e747a6eff700398807fa4035203ef2 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 22 Aug 2025 19:18:27 +0300 Subject: [PATCH 08/10] ngclient: Fix the lockfile handling in Windows There does not seem to be a way around a ugly loop over open()... Signed-off-by: Jussi Kukkonen --- tests/refresh_script.py | 9 ++++- tests/test_updater_ng.py | 49 +++++++++++------------ tests/test_updater_validation.py | 4 +- tox.ini | 4 +- tuf/ngclient/updater.py | 67 +++++++++++++++++++++++--------- 5 files changed, 82 insertions(+), 51 deletions(-) diff --git a/tests/refresh_script.py b/tests/refresh_script.py index ea87d10fd6..605c6bde33 100644 --- a/tests/refresh_script.py +++ b/tests/refresh_script.py @@ -1,4 +1,5 @@ import sys +import time from tuf.ngclient import Updater @@ -6,11 +7,17 @@ print(f" metadata dir: {sys.argv[2]}") print(f" metadata url: {sys.argv[3]}") +start = time.time() for i in range(int(sys.argv[1])): try: + refresh_start = time.time() u = Updater(metadata_dir=sys.argv[2], metadata_base_url=sys.argv[3]) # file3.txt is delegated so we end up exercising all metadata load paths u.get_targetinfo("file3.txt") except OSError as e: - sys.exit(f"Failed on iteration {i}: {e}") + print( + f"Failed on iteration {i}, " + f"{time.time() - refresh_start} secs elapsed ({time.time() - start} total)" + ) + raise e diff --git a/tests/test_updater_ng.py b/tests/test_updater_ng.py index 554961c053..0aae75ac71 100644 --- a/tests/test_updater_ng.py +++ b/tests/test_updater_ng.py @@ -356,16 +356,14 @@ def test_user_agent(self) -> None: self.assertEqual(ua[:23], "MyApp/1.2.3 python-tuf/") - -class TestParallelUpdater(TestUpdater): def test_parallel_updaters(self) -> None: - # Refresh two updaters in parallel many times, using the same local metadata cache. + # Refresh many updaters in parallel many times, using the same local metadata cache. # This should reveal race conditions. - iterations = 100 + iterations = 50 + process_count = 10 - # The project root is the parent of the tests directory - project_root = os.path.dirname(utils.TESTS_DIR) + project_root_dir = os.path.dirname(utils.TESTS_DIR) command = [ sys.executable, @@ -376,29 +374,26 @@ def test_parallel_updaters(self) -> None: self.metadata_url, ] - p1 = subprocess.Popen( - command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=project_root, - ) - p2 = subprocess.Popen( - command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=project_root, - ) - - stdout1, stderr1 = p1.communicate() - stdout2, stderr2 = p2.communicate() + procs = [ + subprocess.Popen( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + cwd=project_root_dir, + ) + for _ in range(process_count) + ] - if p1.returncode != 0 or p2.returncode != 0: + errout = "" + for proc in procs: + stdout, stderr = proc.communicate() + if proc.returncode != 0: + errout += "Parallel Refresh script failed:" + errout += f"\nprocess stdout: \n{stdout.decode()}" + errout += f"\nprocess stderr: \n{stderr.decode()}" + if errout: self.fail( - "Parallel refresh failed" - f"\nprocess 1 stdout: \n{stdout1.decode()}" - f"\nprocess 1 stderr: \n{stderr1.decode()}" - f"\nprocess 2 stdout: \n{stdout2.decode()}" - f"\nprocess 2 stderr: \n{stderr2.decode()}" + f"One or more scripts failed parallel refresh test:\n{errout}" ) diff --git a/tests/test_updater_validation.py b/tests/test_updater_validation.py index b9d6bb3cc7..8020e69de9 100644 --- a/tests/test_updater_validation.py +++ b/tests/test_updater_validation.py @@ -53,9 +53,9 @@ def test_local_target_storage_fail(self) -> None: def test_non_existing_metadata_dir(self) -> None: with self.assertRaises(FileNotFoundError): - # Initialize Updater with non-existing metadata_dir + # Initialize Updater with non-existing metadata_dir and no bootstrap root Updater( - "non_existing_metadata_dir", + f"{self.temp_dir.name}/non_existing_metadata_dir", "https://example.com/metadata/", fetcher=self.sim, ) diff --git a/tox.ini b/tox.ini index 7ef098ba3c..edb657172a 100644 --- a/tox.ini +++ b/tox.ini @@ -11,8 +11,8 @@ skipsdist = true [testenv] commands = python3 --version - python3 -m coverage run -m unittest - python3 -m coverage report -m --fail-under 97 + python3 -m coverage run -m unittest -v + python3 -m coverage report -m --fail-under 96 deps = -r{toxinidir}/requirements/test.txt diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index cfac656926..7555de880d 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -58,6 +58,7 @@ import os import shutil import tempfile +import time from pathlib import Path from typing import IO, TYPE_CHECKING, cast from urllib import parse @@ -79,21 +80,50 @@ # advisory file locking for posix import fcntl - def _lock_file(f: IO) -> None: - if f.writable(): + @contextlib.contextmanager + def _lock_file(path: str) -> Iterator[IO]: + with open(path, "wb") as f: fcntl.lockf(f, fcntl.LOCK_EX) + yield f except ModuleNotFoundError: - # Windows file locking + # Windows file locking, in belt-and-suspenders-from-Temu style: + # Use a loop that tries to open the lockfile for 30 secs, but also + # use msvcrt.locking(). + # * since open() usually just fails when another process has the file open + # msvcrt.locking() almost never gets called when there is a lock. open() + # sometimes succeeds for multiple processes though + # * msvcrt.locking() does not even block until file is available: it just + # tries once per second in a non-blocking manner for 10 seconds. So if + # another process keeps opening the file it's unlikely that we actually + # get the lock import msvcrt - def _lock_file(f: IO) -> None: - # On Windows we lock a byte range and file must not be empty - f.write(b"\0") - f.flush() - f.seek(0) - - msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) + @contextlib.contextmanager + def _lock_file(path: str) -> Iterator[IO]: + err = None + locked = False + for _ in range(100): + try: + with open(path, "wb") as f: + msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) + locked = True + yield f + return + except FileNotFoundError: + # could be from yield or from open() -- either way we bail + raise + except OSError as e: + if locked: + # yield has raised, let's not continue loop + raise e + err = e + logger.warning("Unsuccessful lock attempt for %s: %s", path, e) + time.sleep(0.3) + + # raise the last failure if we never got a lock + if err is not None: + raise err class Updater: @@ -153,6 +183,10 @@ def __init__( f"got '{self.config.envelope_type}'" ) + # Ensure the whole metadata directory structure exists + rootdir = Path(self._dir, "root_history") + rootdir.mkdir(exist_ok=True, parents=True) + with self._lock_metadata(): if not bootstrap: # if no root was provided, use the cached non-versioned root @@ -168,15 +202,11 @@ def __init__( @contextlib.contextmanager def _lock_metadata(self) -> Iterator[None]: """Context manager for locking the metadata directory.""" - # Ensure the whole metadata directory structure exists - rootdir = Path(self._dir, "root_history") - rootdir.mkdir(exist_ok=True, parents=True) - with open(os.path.join(self._dir, ".lock"), "wb") as f: - logger.debug("Getting metadata lock...") - _lock_file(f) + logger.debug("Getting metadata lock...") + with _lock_file(os.path.join(self._dir, ".lock")): yield - logger.debug("Releasing metadata lock") + logger.debug("Released metadata lock") def refresh(self) -> None: """Refresh top-level metadata. @@ -337,8 +367,7 @@ def download_target( targetinfo.verify_length_and_hashes(target_file) target_file.seek(0) - with open(filepath, "wb") as destination_file: - _lock_file(destination_file) + with _lock_file(filepath) as destination_file: shutil.copyfileobj(target_file, destination_file) logger.debug("Downloaded target %s", targetinfo.path) From 5f467bba581c1c3db3abc780abb9eae9ecc5d03d Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Sun, 24 Aug 2025 16:21:28 +0300 Subject: [PATCH 09/10] ngclient: Refactor lock file implementation Signed-off-by: Jussi Kukkonen --- tuf/ngclient/_internal/file_lock.py | 56 +++++++++++++++++++++++++++ tuf/ngclient/updater.py | 59 +++-------------------------- 2 files changed, 61 insertions(+), 54 deletions(-) create mode 100644 tuf/ngclient/_internal/file_lock.py diff --git a/tuf/ngclient/_internal/file_lock.py b/tuf/ngclient/_internal/file_lock.py new file mode 100644 index 0000000000..d970c17531 --- /dev/null +++ b/tuf/ngclient/_internal/file_lock.py @@ -0,0 +1,56 @@ +import logging +import time +from collections.abc import Iterator +from contextlib import contextmanager +from typing import IO + +logger = logging.getLogger(__name__) + +try: + # advisory file locking for posix + import fcntl + + @contextmanager + def lock_file(path: str) -> Iterator[IO]: + with open(path, "wb") as f: + fcntl.lockf(f, fcntl.LOCK_EX) + yield f + +except ModuleNotFoundError: + # Windows file locking, in belt-and-suspenders-from-Temu style: + # Use a loop that tries to open the lockfile for 30 secs, but also + # use msvcrt.locking(). + # * since open() usually just fails when another process has the file open + # msvcrt.locking() almost never gets called when there is a lock. open() + # sometimes succeeds for multiple processes though + # * msvcrt.locking() does not even block until file is available: it just + # tries once per second in a non-blocking manner for 10 seconds. So if + # another process keeps opening the file it's unlikely that we actually + # get the lock + import msvcrt + + @contextmanager + def lock_file(path: str) -> Iterator[IO]: + err = None + locked = False + for _ in range(100): + try: + with open(path, "wb") as f: + msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) + locked = True + yield f + return + except FileNotFoundError: + # could be from yield or from open() -- either way we bail + raise + except OSError as e: + if locked: + # yield has raised, let's not continue loop + raise e + err = e + logger.warning("Unsuccessful lock attempt for %s: %s", path, e) + time.sleep(0.3) + + # raise the last failure if we never got a lock + if err is not None: + raise err diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index 7555de880d..605f14dd43 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -58,13 +58,13 @@ import os import shutil import tempfile -import time from pathlib import Path -from typing import IO, TYPE_CHECKING, cast +from typing import TYPE_CHECKING, cast from urllib import parse from tuf.api import exceptions from tuf.api.metadata import Root, Snapshot, TargetFile, Targets, Timestamp +from tuf.ngclient._internal.file_lock import lock_file from tuf.ngclient._internal.trusted_metadata_set import TrustedMetadataSet from tuf.ngclient.config import EnvelopeType, UpdaterConfig from tuf.ngclient.urllib3_fetcher import Urllib3Fetcher @@ -76,55 +76,6 @@ logger = logging.getLogger(__name__) -try: - # advisory file locking for posix - import fcntl - - @contextlib.contextmanager - def _lock_file(path: str) -> Iterator[IO]: - with open(path, "wb") as f: - fcntl.lockf(f, fcntl.LOCK_EX) - yield f - -except ModuleNotFoundError: - # Windows file locking, in belt-and-suspenders-from-Temu style: - # Use a loop that tries to open the lockfile for 30 secs, but also - # use msvcrt.locking(). - # * since open() usually just fails when another process has the file open - # msvcrt.locking() almost never gets called when there is a lock. open() - # sometimes succeeds for multiple processes though - # * msvcrt.locking() does not even block until file is available: it just - # tries once per second in a non-blocking manner for 10 seconds. So if - # another process keeps opening the file it's unlikely that we actually - # get the lock - import msvcrt - - @contextlib.contextmanager - def _lock_file(path: str) -> Iterator[IO]: - err = None - locked = False - for _ in range(100): - try: - with open(path, "wb") as f: - msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) - locked = True - yield f - return - except FileNotFoundError: - # could be from yield or from open() -- either way we bail - raise - except OSError as e: - if locked: - # yield has raised, let's not continue loop - raise e - err = e - logger.warning("Unsuccessful lock attempt for %s: %s", path, e) - time.sleep(0.3) - - # raise the last failure if we never got a lock - if err is not None: - raise err - class Updater: """Creates a new ``Updater`` instance and loads trusted root metadata. @@ -204,7 +155,7 @@ def _lock_metadata(self) -> Iterator[None]: """Context manager for locking the metadata directory.""" logger.debug("Getting metadata lock...") - with _lock_file(os.path.join(self._dir, ".lock")): + with lock_file(os.path.join(self._dir, ".lock")): yield logger.debug("Released metadata lock") @@ -274,7 +225,7 @@ def get_targetinfo(self, target_path: str) -> TargetFile | None: with self._lock_metadata(): if Targets.type not in self._trusted_set: - # refresh + # implicit refresh self._load_root() self._load_timestamp() self._load_snapshot() @@ -367,7 +318,7 @@ def download_target( targetinfo.verify_length_and_hashes(target_file) target_file.seek(0) - with _lock_file(filepath) as destination_file: + with lock_file(filepath) as destination_file: shutil.copyfileobj(target_file, destination_file) logger.debug("Downloaded target %s", targetinfo.path) From 55dbb53a5b981f12f5e8d065f043b3036b794814 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Sun, 24 Aug 2025 16:22:44 +0300 Subject: [PATCH 10/10] ngclient: Remove the mention of "single instance" The file locking should make multiple processes safe Signed-off-by: Jussi Kukkonen --- tuf/ngclient/updater.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index 605f14dd43..e96192a16d 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -29,10 +29,6 @@ * ``Updater.download_target()`` downloads a target file and ensures it is verified correct by the metadata. -Note that applications using ``Updater`` should be 'single instance' -applications: running multiple instances that use the same cache directories at -the same time is not supported. - A simple example of using the Updater to implement a Python TUF client that downloads target files is available in `examples/client `_.