From 73f4f102a1beced7c808818fcc333041ded0e8fe Mon Sep 17 00:00:00 2001 From: Ahtisahm Shahid Date: Wed, 16 Sep 2026 15:16:07 +0500 Subject: [PATCH 1/2] fix: give each course import its own working directory Every import of a given course used a single working directory derived from the course key alone, shared by the import task and by both upload views. Two imports of the same course therefore overwrote each other's archive, and whichever import finished first deleted the other's extracted OLX mid-run. The surviving task failed with a bare filesystem error: "[Errno 116] Stale file handle" on an NFS-backed GITHUB_REPO_ROOT, "[Errno 2]" on a local disk. Add course_import_working_dir() and remove_course_import_working_dir(), and use them from import_olx and from both upload views so that every import and every upload gets a private subdirectory. Both upload views now drop their staging copy once the archive is in storage; previously they relied on the import task's cleanup of the shared directory. Cleanup no longer raises, so a failing cleanup can no longer mask the error that triggered it. --- .../contentstore/api/views/course_import.py | 45 ++++--- cms/djangoapps/contentstore/tasks.py | 46 +++++-- .../contentstore/views/import_export.py | 46 +++++-- .../views/tests/test_import_export.py | 123 +++++++++++++++++- 4 files changed, 217 insertions(+), 43 deletions(-) diff --git a/cms/djangoapps/contentstore/api/views/course_import.py b/cms/djangoapps/contentstore/api/views/course_import.py index f2f8f168df50..17152941a922 100644 --- a/cms/djangoapps/contentstore/api/views/course_import.py +++ b/cms/djangoapps/contentstore/api/views/course_import.py @@ -3,14 +3,12 @@ """ -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 @@ -18,7 +16,12 @@ 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) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 73f111ff8462..ab6f78d21782 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/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 diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index e4cc5a04cd17..ea5451c6b86a 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/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.') diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 9f0e7e0a9c35..35fc005b66ee 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -1,6 +1,7 @@ """ Unit tests for course import and export """ +import base64 import copy import itertools import json @@ -21,6 +22,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.core.exceptions import SuspiciousOperation +from django.core.files import File from django.core.files.storage import FileSystemStorage from django.test.utils import override_settings from milestones.tests.utils import MilestonesTestCaseMixin @@ -30,11 +32,13 @@ from rest_framework import status from rest_framework.test import APIClient from storages.backends.s3boto3 import S3Boto3Storage -from user_tasks.models import UserTaskStatus +from user_tasks.models import UserTaskArtifact, UserTaskStatus from cms.djangoapps.contentstore import errors as import_error +from cms.djangoapps.contentstore import tasks from cms.djangoapps.contentstore.api.tests.base import BaseCourseViewTest from cms.djangoapps.contentstore.storage import course_import_export_storage +from cms.djangoapps.contentstore.tasks import import_olx from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url @@ -748,6 +752,123 @@ def test_import_status_response_is_not_cached(self, fmt): self.assertEqual(resp.headers['Cache-Control'], 'no-cache, no-store, must-revalidate') # noqa: PT009 +@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) +class ConcurrentImportTestCase(CourseTestCase): + """ + Tests for two imports of the same course overlapping in time. + + Regression tests for a bug where every import of a given course shared one + working directory, so whichever import finished first deleted the other's + archive and extracted OLX mid-run. The surviving task then failed with a + bare filesystem error (`[Errno 116] Stale file handle` on NFS-backed + ``GITHUB_REPO_ROOT``, `[Errno 2]` on a local disk). + """ + ARCHIVE_NAME = 'course.tar.gz' + + def setUp(self): + super().setUp() + self.content_dir = path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.content_dir, True) + + source_dir = tempfile.mkdtemp(dir=self.content_dir) + os.makedirs(os.path.join(source_dir, 'course')) + with open(os.path.join(source_dir, 'course.xml'), 'w') as course_xml: + course_xml.write('') + with open(os.path.join(source_dir, 'course', '2013_Spring.xml'), 'w') as run_xml: + run_xml.write('') + + self.archive = os.path.join(self.content_dir, self.ARCHIVE_NAME) + with tarfile.open(self.archive, 'w:gz') as archive: + archive.add(source_dir, arcname='exported_course') + + data_root = path(settings.GITHUB_REPO_ROOT) + if not data_root.isdir(): + os.makedirs(data_root) + self.per_course_dir = data_root / base64.urlsafe_b64encode( + repr(self.course.id).encode('utf-8') + ).decode('utf-8') + self.addCleanup(shutil.rmtree, self.per_course_dir, True) + + def stage_upload(self): + """Park a copy of the archive in storage, the way the upload view does.""" + with open(self.archive, 'rb') as archive: + return course_import_export_storage.save( + 'olx_import/' + self.ARCHIVE_NAME, File(archive) + ) + + def run_import(self, storage_path): + """Run one import_olx task to completion (celery is eager under test).""" + return import_olx.delay( + self.user.id, str(self.course.id), storage_path, self.ARCHIVE_NAME, 'en' + ) + + @staticmethod + def status_of(result): + """The UserTaskStatus recorded for a finished task.""" + return UserTaskStatus.objects.get(task_id=result.id) + + @staticmethod + def error_of(status): + """The error message a failed task showed the user, if any.""" + artifact = UserTaskArtifact.objects.filter(status=status, name='Error').first() + return artifact.text if artifact else None + + def test_concurrent_imports_of_same_course_both_succeed(self): + """ + An import that finishes while a second one is still unpacking must not + disturb it. + + Only the timing is simulated: the first import is run at the exact + moment the second one reaches its extraction step. Every filesystem + effect is real. + """ + first_upload = self.stage_upload() + second_upload = self.stage_upload() + self.assertNotEqual(first_upload, second_upload) # noqa: PT009 + + real_extractall = tasks.safe_extractall + observed = {} + + def extract_with_a_concurrent_import(file_name, output_path): + """Runs as the second import extracts; the first import lands here.""" + if not observed.get('fired'): + observed['fired'] = True # the first import must not re-enter + observed['archive_before'] = os.path.exists(file_name) + observed['first_result'] = self.run_import(first_upload) + observed['archive_after'] = os.path.exists(file_name) + observed['dir_after'] = os.path.isdir(output_path) + return real_extractall(file_name, output_path) + + with patch.object(tasks, 'safe_extractall', extract_with_a_concurrent_import): + second_result = self.run_import(second_upload) + + first_status = self.status_of(observed['first_result']) + second_status = self.status_of(second_result) + + self.assertEqual(first_status.state, UserTaskStatus.SUCCEEDED) # noqa: PT009 + # The first import's cleanup left the second import's files alone. + self.assertTrue(observed['archive_before']) # noqa: PT009 + self.assertTrue(observed['archive_after']) # noqa: PT009 + self.assertTrue(observed['dir_after']) # noqa: PT009 + self.assertEqual( # noqa: PT009 + second_status.state, UserTaskStatus.SUCCEEDED, self.error_of(second_status) + ) + # ...and between them they left no scratch data behind. + self.assertFalse(os.path.exists(self.per_course_dir)) # noqa: PT009 + + def test_import_working_dirs_are_not_shared(self): + """Two imports of one course never get the same working directory.""" + self.assertNotEqual( # noqa: PT009 + tasks.course_import_working_dir(self.course.id, 'task-1'), + tasks.course_import_working_dir(self.course.id, 'task-2'), + ) + self.assertTrue( # noqa: PT009 + tasks.course_import_working_dir(self.course.id, 'task-1').startswith( + self.per_course_dir + ) + ) + + @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) @ddt.ddt class ExportTestCase(CourseTestCase): From b3d52d12121b7237af401a7b92caf779613fba5f Mon Sep 17 00:00:00 2001 From: Ahtisahm Shahid Date: Wed, 16 Sep 2026 16:14:28 +0500 Subject: [PATCH 2/2] fix: rename test helper arg that shadowed the rest_framework status import --- cms/djangoapps/contentstore/views/tests/test_import_export.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 35fc005b66ee..9d072e4fe457 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -808,9 +808,9 @@ def status_of(result): return UserTaskStatus.objects.get(task_id=result.id) @staticmethod - def error_of(status): + def error_of(task_status): """The error message a failed task showed the user, if any.""" - artifact = UserTaskArtifact.objects.filter(status=status, name='Error').first() + artifact = UserTaskArtifact.objects.filter(status=task_status, name='Error').first() return artifact.text if artifact else None def test_concurrent_imports_of_same_course_both_succeed(self):