Merge branch 'bschubert/fix-atomic-move-git-repo' into 'master'

Fix os.rename in git source element to correctly handle error codes

See merge request BuildStream/buildstream!938

6 files changed, 147 insertions, 31 deletions

diff --git a/buildstream/_artifactcache/cascache.py b/buildstream/_artifactcache/cascache.py
index f07bd24a1..04c26edfa 100644
--- a/buildstream/_artifactcache/cascache.py
+++ b/buildstream/_artifactcache/cascache.py
@@ -24,7 +24,6 @@ import os
 import stat
 import tempfile
 import uuid
-import errno
 from urllib.parse import urlparse
 
 import grpc
@@ -140,17 +139,13 @@ class CASCache():
             checkoutdir = os.path.join(tmpdir, ref)
             self._checkout(checkoutdir, tree)
 
-            os.makedirs(os.path.dirname(dest), exist_ok=True)
             try:
-                os.rename(checkoutdir, dest)
+                utils.move_atomic(checkoutdir, dest)
+            except utils.DirectoryExistsError:
+                # Another process beat us to rename
+                pass
             except OSError as e:
-                # With rename it's possible to get either ENOTEMPTY or EEXIST
-                # in the case that the destination path is a not empty directory.
-                #
-                # If rename fails with these errors, another process beat
-                # us to it so just ignore.
-                if e.errno not in [errno.ENOTEMPTY, errno.EEXIST]:
-                    raise CASError("Failed to extract directory for ref '{}': {}".format(ref, e)) from e
+                raise CASError("Failed to extract directory for ref '{}': {}".format(ref, e)) from e
 
         return originaldest
 
diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index 23e3369b1..18c8be82a 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -86,7 +86,6 @@ This plugin also utilises the following configurable core plugin warnings:
 """
 
 import os
-import errno
 import re
 import shutil
 from collections.abc import Mapping
@@ -97,6 +96,7 @@ from configparser import RawConfigParser
 from buildstream import Source, SourceError, Consistency, SourceFetcher
 from buildstream import utils
 from buildstream.plugin import CoreWarnings
+from buildstream.utils import move_atomic, DirectoryExistsError
 
 GIT_MODULES = '.gitmodules'
 
@@ -141,21 +141,16 @@ class GitMirror(SourceFetcher):
                                  fail="Failed to clone git repository {}".format(url),
                                  fail_temporarily=True)
 
-                # Attempt atomic rename into destination, this will fail if
-                # another process beat us to the punch
                 try:
-                    os.rename(tmpdir, self.mirror)
+                    move_atomic(tmpdir, self.mirror)
+                except DirectoryExistsError:
+                    # Another process was quicker to download this repository.
+                    # Let's discard our own
+                    self.source.status("{}: Discarding duplicate clone of {}"
+                                       .format(self.source, url))
                 except OSError as e:
-
-                    # When renaming and the destination repo already exists, os.rename()
-                    # will fail with ENOTEMPTY, since an empty directory will be silently
-                    # replaced
-                    if e.errno == errno.ENOTEMPTY:
-                        self.source.status("{}: Discarding duplicate clone of {}"
-                                           .format(self.source, url))
-                    else:
-                        raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
-                                          .format(self.source, url, tmpdir, self.mirror, e)) from e
+                    raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
+                                      .format(self.source, url, tmpdir, self.mirror, e)) from e
 
     def _fetch(self, alias_override=None):
         url = self.source.translate_url(self.url,
diff --git a/buildstream/plugins/sources/pip.py b/buildstream/plugins/sources/pip.py
index df2eeb5db..abef1fd0d 100644
--- a/buildstream/plugins/sources/pip.py
+++ b/buildstream/plugins/sources/pip.py
@@ -68,7 +68,6 @@ details on common configuration options for sources.
    The ``pip`` plugin is available since :ref:`format version 16 <project_format_version>`
 """
 
-import errno
 import hashlib
 import os
 import re
@@ -193,13 +192,14 @@ class PipSource(Source):
             # process has fetched the sources before us and ensure that we do
             # not raise an error in that case.
             try:
-                os.makedirs(self._mirror)
-                os.rename(package_dir, self._mirror)
-            except FileExistsError:
-                return
+                utils.move_atomic(package_dir, self._mirror)
+            except utils.DirectoryExistsError:
+                # Another process has beaten us and has fetched the sources
+                # before us.
+                pass
             except OSError as e:
-                if e.errno != errno.ENOTEMPTY:
-                    raise
+                raise SourceError("{}: Failed to move downloaded pip packages from '{}' to '{}': {}"
+                                  .format(self, package_dir, self._mirror, e)) from e
 
     def stage(self, directory):
         with self.timed_activity("Staging Python packages", silent_nested=True):
diff --git a/buildstream/utils.py b/buildstream/utils.py
index dc66f3e62..968d87f7b 100644
--- a/buildstream/utils.py
+++ b/buildstream/utils.py
@@ -72,6 +72,11 @@ class ProgramNotFoundError(BstError):
         super().__init__(message, domain=ErrorDomain.PROG_NOT_FOUND, reason=reason)
 
 
+class DirectoryExistsError(OSError):
+    """Raised when a `os.rename` is attempted but the destination is an existing directory.
+    """
+
+
 class FileListResult():
     """An object which stores the result of one of the operations
     which run on a list of files.
@@ -500,6 +505,38 @@ def get_bst_version():
                         .format(__version__))
 
 
+def move_atomic(source, destination, ensure_parents=True):
+    """Move the source to the destination using atomic primitives.
+
+    This uses `os.rename` to move a file or directory to a new destination.
+    It wraps some `OSError` thrown errors to ensure their handling is correct.
+
+    The main reason for this to exist is that rename can throw different errors
+    for the same symptom (https://www.unix.com/man-page/POSIX/3posix/rename/).
+
+    We are especially interested here in the case when the destination already
+    exists. In this case, either EEXIST or ENOTEMPTY are thrown.
+
+    In order to ensure consistent handling of these exceptions, this function
+    should be used instead of `os.rename`
+
+    Args:
+      source (str or Path): source to rename
+      destination (str or Path): destination to which to move the source
+      ensure_parents (bool): Whether or not to create the parent's directories
+                             of the destination (default: True)
+    """
+    if ensure_parents:
+        os.makedirs(os.path.dirname(str(destination)), exist_ok=True)
+
+    try:
+        os.rename(str(source), str(destination))
+    except OSError as exc:
+        if exc.errno in (errno.EEXIST, errno.ENOTEMPTY):
+            raise DirectoryExistsError(*exc.args) from exc
+        raise
+
+
 @contextmanager
 def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
                      errors=None, newline=None, closefd=True, opener=None, tempdir=None):
diff --git a/tests/frontend/buildtrack.py b/tests/frontend/buildtrack.py
index 3f0a3adbe..720ab7efc 100644
--- a/tests/frontend/buildtrack.py
+++ b/tests/frontend/buildtrack.py
@@ -115,6 +115,7 @@ def test_build_track(cli, datafiles, tmpdir, ref_storage,
     args += ['0.bst']
 
     result = cli.run(project=project, silent=True, args=args)
+    result.assert_success()
     tracked_elements = result.get_tracked_elements()
 
     assert set(tracked_elements) == set(tracked)
diff --git a/tests/utils/movedirectory.py b/tests/utils/movedirectory.py
new file mode 100755
index 000000000..6edbec611
--- /dev/null
+++ b/tests/utils/movedirectory.py
@@ -0,0 +1,88 @@
+import pytest
+
+from buildstream.utils import move_atomic, DirectoryExistsError
+
+
+@pytest.fixture
+def src(tmp_path):
+    src = tmp_path.joinpath("src")
+    src.mkdir()
+
+    with src.joinpath("test").open("w") as fp:
+        fp.write("test")
+
+    return src
+
+
+def test_move_to_empty_dir(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+
+    move_atomic(src, dst)
+
+    assert dst.joinpath("test").exists()
+
+
+def test_move_to_empty_dir_create_parents(src, tmp_path):
+    dst = tmp_path.joinpath("nested/dst")
+
+    move_atomic(src, dst)
+    assert dst.joinpath("test").exists()
+
+
+def test_move_to_empty_dir_no_create_parents(src, tmp_path):
+    dst = tmp_path.joinpath("nested/dst")
+
+    with pytest.raises(FileNotFoundError):
+        move_atomic(src, dst, ensure_parents=False)
+
+
+def test_move_non_existing_dir(tmp_path):
+    dst = tmp_path.joinpath("dst")
+    src = tmp_path.joinpath("src")
+
+    with pytest.raises(FileNotFoundError):
+        move_atomic(src, dst)
+
+
+def test_move_to_existing_empty_dir(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+    dst.mkdir()
+
+    move_atomic(src, dst)
+    assert dst.joinpath("test").exists()
+
+
+def test_move_to_existing_file(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+
+    with dst.open("w") as fp:
+        fp.write("error")
+
+    with pytest.raises(NotADirectoryError):
+        move_atomic(src, dst)
+
+
+def test_move_file_to_existing_file(tmp_path):
+    dst = tmp_path.joinpath("dst")
+    src = tmp_path.joinpath("src")
+
+    with src.open("w") as fp:
+        fp.write("src")
+
+    with dst.open("w") as fp:
+        fp.write("dst")
+
+    move_atomic(src, dst)
+    with dst.open() as fp:
+        assert fp.read() == "src"
+
+
+def test_move_to_existing_non_empty_dir(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+    dst.mkdir()
+
+    with dst.joinpath("existing").open("w") as fp:
+        fp.write("already there")
+
+    with pytest.raises(DirectoryExistsError):
+        move_atomic(src, dst)