fix: give each course import its own working directory

cms/djangoapps/contentstore/api/views/course_import.py

@@ -3,22 +3,25 @@
 """
 
 
-import base64
 import logging
 import os
+from uuid import uuid4
 
-from django.conf import settings
 from django.core.files import File
 from edx_django_utils.monitoring import set_custom_attribute, set_custom_attributes_for_course_key
-from path import Path as path
 from rest_framework import status
 from rest_framework.exceptions import AuthenticationFailed
 from rest_framework.generics import GenericAPIView
 from rest_framework.response import Response
 from user_tasks.models import UserTaskStatus
 
 from cms.djangoapps.contentstore.storage import course_import_export_storage
-from cms.djangoapps.contentstore.tasks import CourseImportTask, import_olx
+from cms.djangoapps.contentstore.tasks import (
+    CourseImportTask,
+    course_import_working_dir,
+    import_olx,
+    remove_course_import_working_dir,
+)
 from cms.djangoapps.contentstore.utils import IMPORTABLE_FILE_TYPES
 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes
 
@@ -131,22 +134,26 @@ def post(self, request, course_key):
                     developer_message='Parameter in the wrong format',
                     error_code='internal_error',
                 )
-            course_dir = path(settings.GITHUB_REPO_ROOT) / base64.urlsafe_b64encode(
-                repr(course_key).encode('utf-8')
-            ).decode('utf-8')
+            # Staging directory private to this upload, so that a concurrent import of
+            # the same course cannot overwrite this archive or delete it on its way out.
+            course_dir = course_import_working_dir(course_key, f'upload-{uuid4().hex}')
             temp_filepath = course_dir / filename
-            if not course_dir.isdir():
-                os.mkdir(course_dir)
-
-            log.debug(f'importing course to {temp_filepath}')
-            with open(temp_filepath, "wb+") as temp_file:
-                for chunk in request.FILES['course_data'].chunks():
-                    temp_file.write(chunk)
-
-            log.info("Course import %s: Upload complete", course_key)
-            with open(temp_filepath, 'rb') as local_file:
-                django_file = File(local_file)
-                storage_path = course_import_export_storage.save('olx_import/' + filename, django_file)
+            os.makedirs(course_dir, exist_ok=True)
+
+            try:
+                log.debug(f'importing course to {temp_filepath}')
+                with open(temp_filepath, "wb+") as temp_file:
+                    for chunk in request.FILES['course_data'].chunks():
+                        temp_file.write(chunk)
+
+                log.info("Course import %s: Upload complete", course_key)
+                with open(temp_filepath, 'rb') as local_file:
+                    django_file = File(local_file)
+                    storage_path = course_import_export_storage.save('olx_import/' + filename, django_file)
+            finally:
+                # The archive now lives in storage; the import task downloads it into its
+                # own working directory, so this staging copy is no longer needed.
+                remove_course_import_working_dir(course_dir)
 
             async_result = import_olx.delay(
                 request.user.id, str(course_key), storage_path, filename, request.LANGUAGE_CODE)

cms/djangoapps/contentstore/tasks.py

@@ -522,6 +522,36 @@ def sync_discussion_settings(course_key, user):
         LOGGER.info(f'Course import {course.id}: DiscussionsConfiguration sync failed: {exc}')
 
 
+def course_import_working_dir(courselike_key, unique_id):
+    """
+    Return a private scratch directory for one course import.
+
+    ``unique_id`` must be unique per import (a task id, an upload id, ...). Every
+    import needs its own directory: they used to share a single directory derived
+    from the course key alone, so two imports of the same course would overwrite
+    each other's archive, and whichever import finished first deleted the other's
+    extracted OLX mid-run.
+    """
+    subdir = base64.urlsafe_b64encode(repr(courselike_key).encode('utf-8')).decode('utf-8')
+    return path(settings.GITHUB_REPO_ROOT) / subdir / str(unique_id)
+
+
+def remove_course_import_working_dir(course_dir):
+    """
+    Delete a working directory created by :func:`course_import_working_dir`.
+
+    Also drops the per-course parent directory once the last import using it is
+    gone. Both steps tolerate the directory already being missing, so that
+    cleanup never masks the error that triggered it.
+    """
+    shutil.rmtree(course_dir, ignore_errors=True)
+    try:
+        os.rmdir(os.path.dirname(course_dir))
+    except OSError:
+        # Still holds another import's working directory, or is already gone.
+        pass
+
+
 @shared_task(base=CourseImportTask, bind=True)
 # Note: The decorator @set_code_owner_attribute cannot be used here because the UserTaskMixin
 #   does stack inspection and can't handle additional decorators.
@@ -538,8 +568,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan
     self.status.set_state(current_step)
 
     data_root = path(settings.GITHUB_REPO_ROOT)
-    subdir = base64.urlsafe_b64encode(repr(courselike_key).encode('utf-8')).decode('utf-8')
-    course_dir = data_root / subdir
+    course_dir = course_import_working_dir(courselike_key, self.request.id)
 
     def validate_user():
         """Validate if the user exists otherwise log error. """
@@ -647,8 +676,7 @@ def get_dir_for_filename(directory, filename):
         LOGGER.info(f'{log_prefix}: unpacking step started')
 
         temp_filepath = course_dir / get_valid_filename(archive_name)
-        if not course_dir.isdir():
-            os.mkdir(course_dir)
+        os.makedirs(course_dir, exist_ok=True)
 
         LOGGER.info(f'{log_prefix}: importing course to {temp_filepath}')
 
@@ -684,9 +712,8 @@ def read_chunk():
                 LOGGER.info(f'{log_prefix}: entrance exam milestone content reference has been removed')
     # Send errors to client with stage at which error occurred.
     except Exception as exception:  # pylint: disable=broad-except
-        if course_dir.isdir():
-            shutil.rmtree(course_dir)
-            LOGGER.info(f'{log_prefix}: Temp data cleared')
+        remove_course_import_working_dir(course_dir)
+        LOGGER.info(f'{log_prefix}: Temp data cleared')
 
         self.status.fail(UserErrors.UNKNOWN_ERROR_IN_UNPACKING)
         LOGGER.exception(f'{log_prefix}: Unknown error while unpacking', exc_info=True)
@@ -742,9 +769,8 @@ def read_chunk():
     except Exception as exception:  # pylint: disable=broad-except
         handle_course_import_exception(courselike_key, exception, self.status, known=False)
     finally:
-        if course_dir.isdir():
-            shutil.rmtree(course_dir)
-            LOGGER.info(f'{log_prefix}: Temp data cleared')
+        remove_course_import_working_dir(course_dir)
+        LOGGER.info(f'{log_prefix}: Temp data cleared')
 
         if self.status.state == 'Updating' and is_course:
             # Reload the course so we have the latest state

cms/djangoapps/contentstore/views/import_export.py

@@ -4,12 +4,11 @@
 """
 
 
-import base64
+import hashlib
 import json
 import logging
 import os
 import re
-import shutil
 from wsgiref.util import FileWrapper
 
 from django.conf import settings
@@ -28,7 +27,6 @@
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locator import LibraryLocator
 from openedx_authz.constants.permissions import COURSES_EXPORT_COURSE, COURSES_IMPORT_COURSE
-from path import Path as path
 from storages.backends.s3boto3 import S3Boto3Storage
 from user_tasks.conf import settings as user_tasks_settings
 from user_tasks.models import UserTaskArtifact, UserTaskStatus
@@ -42,7 +40,15 @@
 from xmodule.modulestore.django import modulestore  # pylint: disable=wrong-import-order
 
 from ..storage import course_import_export_storage
-from ..tasks import CourseExportTask, CourseImportTask, create_export_tarball, export_olx, import_olx
+from ..tasks import (
+    CourseExportTask,
+    CourseImportTask,
+    course_import_working_dir,
+    create_export_tarball,
+    export_olx,
+    import_olx,
+    remove_course_import_working_dir,
+)
 from ..utils import IMPORTABLE_FILE_TYPES, get_export_url, get_import_url, reverse_course_url
 
 __all__ = [
@@ -110,15 +116,28 @@ def _save_request_status(request, key, status):
     request.session.save()
 
 
+def _upload_id(request, filename):
+    """
+    Identify one chunked upload, so that its staging directory is not shared.
+
+    Chunks of a single upload all have to land in the same directory, so the id
+    is derived from the session and the file name rather than being random.
+    """
+    session_key = request.session.session_key or ''
+    digest = hashlib.sha256(f'{session_key}:{filename}'.encode()).hexdigest()
+    return f'upload-{digest[:32]}'
+
+
 def _write_chunk(request, courselike_key):  # pylint: disable=too-many-statements
     """
     Write the OLX file data chunk from the given request to the local filesystem.
     """
-    # Upload .tar.gz or .zip to local filesystem for one-server installations not using S3 or Swift
-    data_root = path(settings.GITHUB_REPO_ROOT)
-    subdir = base64.urlsafe_b64encode(repr(courselike_key).encode('utf-8')).decode('utf-8')
-    course_dir = data_root / subdir
     filename = request.FILES['course-data'].name
+    # Upload .tar.gz or .zip to local filesystem for one-server installations not using S3 or Swift.
+    # The staging directory is private to this upload: it has to survive between chunks of the same
+    # upload, but must not be shared with another author's upload or with a running import task,
+    # which would delete it out from under us on its way out.
+    course_dir = course_import_working_dir(courselike_key, _upload_id(request, filename))
     set_custom_attributes_for_course_key(courselike_key)
     current_step = 'Uploading'
 
@@ -139,8 +158,7 @@ def error_response(message, status, stage):
             return error_response(error_message, 415, 0)
 
         temp_filepath = course_dir / filename
-        if not course_dir.isdir():
-            os.mkdir(course_dir)
+        os.makedirs(course_dir, exist_ok=True)
 
         logging.info(f'Course import {courselike_key}: importing course to {temp_filepath}')
 
@@ -207,15 +225,17 @@ def error_response(message, status, stage):
         with open(temp_filepath, 'rb') as local_file:
             django_file = File(local_file)
             storage_path = course_import_export_storage.save('olx_import/' + filename, django_file)
+        # The archive now lives in storage; the import task downloads it into its own
+        # working directory, so this staging copy is no longer needed.
+        remove_course_import_working_dir(course_dir)
         import_olx.delay(
             request.user.id, str(courselike_key), storage_path, filename, request.LANGUAGE_CODE)
 
     # Send errors to client with stage at which error occurred.
     except Exception as exception:  # pylint: disable=broad-except
         _save_request_status(request, courselike_string, -1)
-        if course_dir.isdir():
-            shutil.rmtree(course_dir)
-            log.info("Course import %s: Temp data cleared", courselike_key)
+        remove_course_import_working_dir(course_dir)
+        log.info("Course import %s: Temp data cleared", courselike_key)
 
         monitor_import_failure(courselike_key, current_step, exception=exception)
         log.exception(f'Course import {courselike_key}: error importing course.')