"conda index" leads to "JSONDecodeError" in "conda install|update"

[sscherfke]

Actual Behavior

Since we’ve upgraded to conda 4.7 and conda-build 3.18, we ran into lots of JSONDecodeErrors in our build pipelines, because the repositories’ current_repodata.json (or other JSON files) could not be decoded. This happened especially often in CI pipelines, when other pipelines were uploading new packages.

Expected Behavior

Package uploading should not corrupt the Conda index for installs/updates in other pipelines.

Analysis and Proposed Fix

The problem above should only exist if the JSON files don’t get updated in an atomic operation.

In a first attempt, I wrapped all conda index calls on our forge and and all HTTP GET requests for .json files with file locks. This fixed the issue (but we ran into deadlocks very quickly 🐱 ).

I then tried to fix the issue directly in conda index. This is the relevant code:

# conda_build/index.py:421+16
def _maybe_write(path, content, write_newline_end=False, content_is_binary=False):
    temp_path = join(gettempdir(), str(uuid4()))

    if not content_is_binary:
        content = ensure_binary(content)
    with open(temp_path, 'wb') as fh:
        fh.write(content)
        if write_newline_end:
            fh.write(b'\n')
    if isfile(path):
        if utils.md5_file(temp_path) == utils.md5_file(path):
            # No need to change mtimes. The contents already match.
            os.unlink(temp_path)
            return False
    # log.info("writing %s", path)
    utils.move_with_fallback(temp_path, path)
    return True

# conda_build/utils.py:591+9
def move_with_fallback(src, dst):
    try:
        shutil.move(src, dst)
    except PermissionError:
        try:
            copy_into(src, dst)
            os.unlink(src)
        except PermissionError:
            log = get_logger(__name__)
            log.debug("Failed to copy/remove path from %s to %s due to permission error" % (src, dst))

The crux of the problem lies in shutil.move() (or the copy_into() fallback):

If the destination is on the current filesystem, then os.rename() is used. Otherwise, src is copied to dst using copy_function and then removed.

The index of our forge lives in $HOME and /tmp is usually a different filesystem, so there is no atomic move op, but a copy into which leads to the error described above.

IMHO, the easiest solution for this would be to place the temporary file next to its destination. That way, we can always do an atomic move:

diff --git a/conda_build/index.py b/conda_build/index.py
index 0a3b7ed9..444cc920 100644
--- a/conda_build/index.py
+++ b/conda_build/index.py
@@ -14,7 +14,6 @@ import os
 from os.path import abspath, basename, getmtime, getsize, isdir, isfile, join, splitext, dirname
 import subprocess
 import sys
-from tempfile import gettempdir
 import time
 from uuid import uuid4

@@ -419,7 +418,7 @@ def _get_jinja2_environment():


 def _maybe_write(path, content, write_newline_end=False, content_is_binary=False):
-    temp_path = join(gettempdir(), str(uuid4()))
+    temp_path = '%s.%s' % (path, uuid4())

     if not content_is_binary:
         content = ensure_binary(content)

This fix seems to work quite well on our own forge. If you wish, I can create a pull request for it.

Output of conda info

     active environment : None
            shell level : 0
       user config file : $HOME/.condarc
 populated config files : $HOME/conda/.condarc
          conda version : 4.8.0
    conda-build version : 3.18.11
         python version : 3.7.5.final.0
       virtual packages : __glibc=2.29
       base environment : $HOME/conda  (writable)
           channel URLs : https://ownforge/conda/stable/linux-64
                          https://ownforge/conda/stable/noarch
          package cache : $HOME/conda/pkgs
                          $HOME/.conda/pkgs
       envs directories : $HOME/conda/envs
                          $HOME/.conda/envs
               platform : linux-64
             user-agent : conda/4.8.0 requests/2.22.0 CPython/3.7.5 Linux/5.3.12-200.fc30.x86_64 fedora/30 glibc/2.29
                UID:GID : 1119:1119
             netrc file : None
           offline mode : False

[msarahan]

Thanks for the thorough analysis. Your fix seems totally reasonable. A PR would be much appreciated.

…

On Wed, Dec 18, 2019, 04:26 Stefan Scherfke ***@***.***> wrote: Actual Behavior Since we’ve upgraded to conda 4.7 and conda-build 3.18, we ran into lots of JSONDecodeErrors in our build pipelines, because the repositories’ current_repodata.json (or other JSON files) could not be decoded. This happened especially often in CI pipelines, when other pipelines were uploading new packages. Expected Behavior Package uploading should not corrupt the Conda index for installs/updates in other pipelines. Analysis and Proposed Fix The problem above should only exist if the JSON files don’t get updated in an atomic operation. In a first attempt, I wrapped all conda index calls on our forge and and all HTTP GET requests for .json files with file locks. This fixed the issue (but we ran into deadlocks very quickly 🐱 ). I then tried to fix the issue directly in conda index. This is the relevant code: # conda_build/index.py:421+16 def _maybe_write(path, content, write_newline_end=False, content_is_binary=False): temp_path = join(gettempdir(), str(uuid4())) if not content_is_binary: content = ensure_binary(content) with open(temp_path, 'wb') as fh: fh.write(content) if write_newline_end: fh.write(b'\n') if isfile(path): if utils.md5_file(temp_path) == utils.md5_file(path): # No need to change mtimes. The contents already match. os.unlink(temp_path) return False # log.info("writing %s", path) utils.move_with_fallback(temp_path, path) return True # conda_build/utils.py:591+9 def move_with_fallback(src, dst): try: shutil.move(src, dst) except PermissionError: try: copy_into(src, dst) os.unlink(src) except PermissionError: log = get_logger(__name__) log.debug("Failed to copy/remove path from %s to %s due to permission error" % (src, dst)) The crux of the problem lies in shutil.move() <https://docs.python.org/3/library/shutil.html#shutil.move> (or the copy_into() fallback): If the destination is on the current filesystem, then os.rename() is used. Otherwise, src is copied to dst using copy_function and then removed. The index of our forge lives in $HOME and /tmp is usually a different filesystem, so there is no *atomic move* op, but a *copy into* which leads to the error described above. IMHO, the easiest solution for this would be to place the temporary file next to its destination. That way, we can always do an atomic move: diff --git a/conda_build/index.py b/conda_build/index.py index 0a3b7ed9..444cc920 100644 --- a/conda_build/index.py +++ b/conda_build/index.py @@ -14,7 +14,6 @@ import os from os.path import abspath, basename, getmtime, getsize, isdir, isfile, join, splitext, dirname import subprocess import sys -from tempfile import gettempdir import time from uuid import uuid4 @@ -419,7 +418,7 @@ def _get_jinja2_environment(): def _maybe_write(path, content, write_newline_end=False, content_is_binary=False): - temp_path = join(gettempdir(), str(uuid4())) + temp_path = '%s.%s' % (path, uuid4()) if not content_is_binary: content = ensure_binary(content) This fix seems to work quite well on our own forge. If you wish, I can create a pull request for it. Output of conda info active environment : None shell level : 0 user config file : $HOME/.condarc populated config files : $HOME/conda/.condarc conda version : 4.8.0 conda-build version : 3.18.11 python version : 3.7.5.final.0 virtual packages : __glibc=2.29 base environment : $HOME/conda (writable) channel URLs : https://ownforge/conda/stable/linux-64 https://ownforge/conda/stable/noarch package cache : $HOME/conda/pkgs $HOME/.conda/pkgs envs directories : $HOME/conda/envs $HOME/.conda/envs platform : linux-64 user-agent : conda/4.8.0 requests/2.22.0 CPython/3.7.5 Linux/5.3.12-200.fc30.x86_64 fedora/30 glibc/2.29 UID:GID : 1119:1119 netrc file : None offline mode : False — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3833?email_source=notifications&email_token=AAAJL6LFWN64N3MGKBTJCLTQZH3FFA5CNFSM4J4IXBV2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IBJP7YA>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJL6M5YOJ754QE5M5HCALQZH3FFANCNFSM4J4IXBVQ> .

[sscherfke]

I think instead of calling utils.move_with_fallback() we can safely call os.rename() in this case. We cannot get rid of move_with_fallback() completely, though, since it is used in a few other locations, too.

[github-actions]

Hi there, thank you for your contribution!

This issue has been automatically locked because it has not had recent activity after being closed.

Please open a new issue if needed.

Thanks!