From 1d877b44d7d8996304714fdc0acadff08667a56b Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Thu, 21 Dec 2023 00:11:51 +0100 Subject: [PATCH 1/7] Make ensure_path_nonexistent race condition aware --- src/batou/lib/file.py | 24 +++++++++++++++++++----- src/batou/lib/tests/test_file.py | 9 +++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/batou/lib/file.py b/src/batou/lib/file.py index 980e3c59..999d9f80 100644 --- a/src/batou/lib/file.py +++ b/src/batou/lib/file.py @@ -19,14 +19,28 @@ def ensure_path_nonexistent(path): + """Ensure that the given path does not exist. + + strategy: rename the file to a temporary name, then remove it. + this allows us to fail in a useful way, since rename is atomic. + If we detect that the file exists after the rename, we know that + something else is going on and we can bail out. + """ + tmp_nonce = random.getrandbits(32) + tmp_filename = path + ".batou-ensure-path-nonexistent-tmp" + str(tmp_nonce) if not os.path.lexists(path): return - if os.path.islink(path): - os.unlink(path) - elif os.path.isdir(path): - shutil.rmtree(path) + os.rename(path, tmp_filename) + # if the rename did not work, we bail out + assert not os.path.lexists( + path + ), f"{path} should not exist, if it does, it is a race condition." + if os.path.islink(tmp_filename): + os.unlink(tmp_filename) + elif os.path.isdir(tmp_filename): + shutil.rmtree(tmp_filename) else: - os.unlink(path) + os.unlink(tmp_filename) class File(Component): diff --git a/src/batou/lib/tests/test_file.py b/src/batou/lib/tests/test_file.py index 841611b6..4c8d01f3 100644 --- a/src/batou/lib/tests/test_file.py +++ b/src/batou/lib/tests/test_file.py @@ -74,6 +74,15 @@ def test_ensure_path_does_not_fail_on_nonexisting_path(): assert not os.path.exists("missing") +def test_ensure_path_nonexistent_fails_if_os_rename_is_dummied(tmpdir): + os.chdir(str(tmpdir)) + os.mkdir("dir") + with patch("os.rename", Mock(return_value=None)): + with pytest.raises(AssertionError) as e: + ensure_path_nonexistent("dir") + assert "race condition" in str(e.value) + + def test_presence_creates_nonexisting_file(root): p = Presence("path") root.component += p From 7a228e0ae2937a7956cb42756e0effb7e058e49b Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Thu, 21 Dec 2023 00:52:47 +0100 Subject: [PATCH 2/7] add import to random --- src/batou/lib/file.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/batou/lib/file.py b/src/batou/lib/file.py index 999d9f80..f445b3c9 100644 --- a/src/batou/lib/file.py +++ b/src/batou/lib/file.py @@ -5,6 +5,7 @@ import json import os.path import pwd +import random import re import shutil import stat @@ -44,7 +45,6 @@ def ensure_path_nonexistent(path): class File(Component): - namevar = "path" ensure = "file" # or: directory, symlink @@ -157,13 +157,11 @@ def last_updated(self, key="st_mtime"): class BinaryFile(File): - is_template = False encoding = None class Presence(Component): - namevar = "path" leading = False @@ -197,7 +195,6 @@ def last_updated(self, key="st_mtime"): class SyncDirectory(Component): - namevar = "path" source = None exclude = () @@ -252,7 +249,6 @@ def namevar_for_breadcrumb(self): class Directory(Component): - namevar = "path" leading = False source = None @@ -305,7 +301,6 @@ def namevar_for_breadcrumb(self): class FileComponent(Component): - namevar = "path" leading = False @@ -548,7 +543,6 @@ def render(self): class JSONContent(ManagedContentBase): - # Data to be used. data = None @@ -583,7 +577,6 @@ def render(self): class YAMLContent(ManagedContentBase): - # Data to be used. data = None @@ -602,7 +595,6 @@ def render(self): class Owner(FileComponent): - owner = None def verify(self): @@ -617,7 +609,6 @@ def update(self): class Group(FileComponent): - group = None def verify(self): @@ -662,7 +653,6 @@ def convert_mode(string: str) -> int: class Mode(FileComponent): - mode = Attribute(default=None) def configure(self): @@ -709,7 +699,6 @@ def _select_stat_implementation(self): class Symlink(Component): - namevar = "target" source = None From 2eb3d0fcd43c9c9f4a7c16536b67a4073562976d Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Wed, 4 Jan 2023 18:30:25 +0100 Subject: [PATCH 3/7] ensure_path_nonexistent: use tempfile --- src/batou/lib/file.py | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/batou/lib/file.py b/src/batou/lib/file.py index f445b3c9..b65f8224 100644 --- a/src/batou/lib/file.py +++ b/src/batou/lib/file.py @@ -5,7 +5,6 @@ import json import os.path import pwd -import random import re import shutil import stat @@ -19,29 +18,23 @@ from batou.utils import dict_merge -def ensure_path_nonexistent(path): +def ensure_path_nonexistent(path: str) -> None: """Ensure that the given path does not exist. - strategy: rename the file to a temporary name, then remove it. - this allows us to fail in a useful way, since rename is atomic. - If we detect that the file exists after the rename, we know that - something else is going on and we can bail out. + strategy: create a temporary directory, move the path into it + and delete the temporary directory. """ - tmp_nonce = random.getrandbits(32) - tmp_filename = path + ".batou-ensure-path-nonexistent-tmp" + str(tmp_nonce) - if not os.path.lexists(path): + if not os.path.exists(path): return - os.rename(path, tmp_filename) - # if the rename did not work, we bail out - assert not os.path.lexists( - path - ), f"{path} should not exist, if it does, it is a race condition." - if os.path.islink(tmp_filename): - os.unlink(tmp_filename) - elif os.path.isdir(tmp_filename): - shutil.rmtree(tmp_filename) - else: - os.unlink(tmp_filename) + + parent_dir = os.path.dirname(os.path.abspath(path)) + path_basename = os.path.basename(os.path.normpath(path)) + + with tempfile.TemporaryDirectory(dir=parent_dir) as temp_dir: + os.rename(path, os.path.join(temp_dir, path_basename)) + # deletes temp_dir on context manager exit + + assert not os.path.exists(path), f"{path} still exists after rename, race?" class File(Component): From c243cd6fac8fa50d585d400802d095a248f22731 Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Mon, 8 Jan 2024 20:53:51 +0100 Subject: [PATCH 4/7] adjust test for race condition --- src/batou/lib/tests/test_file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/batou/lib/tests/test_file.py b/src/batou/lib/tests/test_file.py index 4c8d01f3..cedd1b46 100644 --- a/src/batou/lib/tests/test_file.py +++ b/src/batou/lib/tests/test_file.py @@ -80,7 +80,7 @@ def test_ensure_path_nonexistent_fails_if_os_rename_is_dummied(tmpdir): with patch("os.rename", Mock(return_value=None)): with pytest.raises(AssertionError) as e: ensure_path_nonexistent("dir") - assert "race condition" in str(e.value) + assert "race" in str(e.value) def test_presence_creates_nonexisting_file(root): From 8deb1106a4eb9407c930873fdc3beb1127c0809f Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Tue, 9 Jan 2024 03:29:18 +0100 Subject: [PATCH 5/7] fix tests by checking for broken symlinks and readd newlines remove by black 2023 --- src/batou/lib/file.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/batou/lib/file.py b/src/batou/lib/file.py index b65f8224..f3059866 100644 --- a/src/batou/lib/file.py +++ b/src/batou/lib/file.py @@ -24,7 +24,7 @@ def ensure_path_nonexistent(path: str) -> None: strategy: create a temporary directory, move the path into it and delete the temporary directory. """ - if not os.path.exists(path): + if not os.path.lexists(path): return parent_dir = os.path.dirname(os.path.abspath(path)) @@ -38,6 +38,7 @@ def ensure_path_nonexistent(path: str) -> None: class File(Component): + namevar = "path" ensure = "file" # or: directory, symlink @@ -150,11 +151,13 @@ def last_updated(self, key="st_mtime"): class BinaryFile(File): + is_template = False encoding = None class Presence(Component): + namevar = "path" leading = False @@ -188,6 +191,7 @@ def last_updated(self, key="st_mtime"): class SyncDirectory(Component): + namevar = "path" source = None exclude = () @@ -242,6 +246,7 @@ def namevar_for_breadcrumb(self): class Directory(Component): + namevar = "path" leading = False source = None @@ -294,6 +299,7 @@ def namevar_for_breadcrumb(self): class FileComponent(Component): + namevar = "path" leading = False @@ -536,6 +542,7 @@ def render(self): class JSONContent(ManagedContentBase): + # Data to be used. data = None @@ -570,6 +577,7 @@ def render(self): class YAMLContent(ManagedContentBase): + # Data to be used. data = None @@ -588,6 +596,7 @@ def render(self): class Owner(FileComponent): + owner = None def verify(self): @@ -602,6 +611,7 @@ def update(self): class Group(FileComponent): + group = None def verify(self): @@ -646,6 +656,7 @@ def convert_mode(string: str) -> int: class Mode(FileComponent): + mode = Attribute(default=None) def configure(self): @@ -692,6 +703,7 @@ def _select_stat_implementation(self): class Symlink(Component): + namevar = "target" source = None From 405f97bce528f97d148446d685720262405e3b8e Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Wed, 24 Jan 2024 16:29:13 +0100 Subject: [PATCH 6/7] Add docs, fix ensure_path_nonexistent --- src/batou/lib/file.py | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/batou/lib/file.py b/src/batou/lib/file.py index f3059866..8d7de1ef 100644 --- a/src/batou/lib/file.py +++ b/src/batou/lib/file.py @@ -17,13 +17,28 @@ from batou.component import Attribute, Component from batou.utils import dict_merge +# Explain the logic that in a multi-user and concurrent system there isn't really a coarse grained guarantee around files not existing. + +# What this can guarantee, however, is that during the lifetime of this function call, there will have been an atomic moment where the file did not exist, or, if it existed and can not be removed, it will raise an error. + +# This means: if the file wasn't there in the first place, we're fine. If the file existed and we removed it, we are fine, we don't care whether someone else might have recreated it immediately after. If the file existed and was removed by someone else, we don't care either. If the file existed and we can't remove it, we need to fail. + def ensure_path_nonexistent(path: str) -> None: """Ensure that the given path does not exist. - strategy: create a temporary directory, move the path into it - and delete the temporary directory. + In a multi-user/concurrent environment, it is possible that a path is created + at any time. This function will ensure that the path does not exist at some + point in time during the execution of this function. + + During the lifetime of this function call, there will have been an atomic + moment where the file did not exist, or, if it existed and can not be + removed, it will raise an error. + + This function will not guarantee that the path does not exist after this + function has returned. """ + if not os.path.lexists(path): return @@ -31,10 +46,11 @@ def ensure_path_nonexistent(path: str) -> None: path_basename = os.path.basename(os.path.normpath(path)) with tempfile.TemporaryDirectory(dir=parent_dir) as temp_dir: - os.rename(path, os.path.join(temp_dir, path_basename)) - # deletes temp_dir on context manager exit - - assert not os.path.exists(path), f"{path} still exists after rename, race?" + try: + os.rename(path, os.path.join(temp_dir, path_basename)) + except FileNotFoundError as e: + # The file was not there in the first place, we're done. + pass class File(Component): From 50b631ad068bc4243f7a74a4ce651bdcb6050be1 Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Wed, 24 Jan 2024 17:22:46 +0100 Subject: [PATCH 7/7] removes a test for ensure_path_nonexistent (race) --- src/batou/lib/tests/test_file.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/batou/lib/tests/test_file.py b/src/batou/lib/tests/test_file.py index cedd1b46..841611b6 100644 --- a/src/batou/lib/tests/test_file.py +++ b/src/batou/lib/tests/test_file.py @@ -74,15 +74,6 @@ def test_ensure_path_does_not_fail_on_nonexisting_path(): assert not os.path.exists("missing") -def test_ensure_path_nonexistent_fails_if_os_rename_is_dummied(tmpdir): - os.chdir(str(tmpdir)) - os.mkdir("dir") - with patch("os.rename", Mock(return_value=None)): - with pytest.raises(AssertionError) as e: - ensure_path_nonexistent("dir") - assert "race" in str(e.value) - - def test_presence_creates_nonexisting_file(root): p = Presence("path") root.component += p