Fix failures during publish stage of course build

Removal of temporary files might block the execution for
tens of seconds if a course has many files in build.
During this time the cached configuration was not
pointing to right paths, and caused failures if accessed.
Fix postpones file removal until the end of build process.

Author: PasiSa

access/views.py

@@ -191,13 +191,15 @@ def aplus_json(request: HttpRequest, course_key: str) -> HttpResponse:
         except ConfigError as e:
             errors.append(f"Failed to load newly built course due to this error: {e}")
             errors.append("Attempting to load previous version of the course...")
+            logger.warn(f"Failed to load newly built course due to this error: {e}")
         else:
             defaults_path = CourseConfig.defaults_path(course_key, source=ConfigSource.STORE)
 
     if config is None:
         try:
             config = CourseConfig.get(course_key, source=ConfigSource.PUBLISH)
         except ConfigError as e:
+            logger.error(f"aplus_json: failed to get config for {course_key}")
             try:
                 Course.objects.get(key=course_key)
             except:

builder/builder.py

@@ -28,7 +28,7 @@
 from access.config import INDEX, ConfigSource, CourseConfig, load_meta, META
 from access.parser import ConfigError
 from builder.configure import configure_graders, publish_graders
-from util.files import is_subpath, renames, rm_path, FileLock
+from util.files import is_subpath, renames, rm_path, rm_paths, FileLock
 from util.git import checkout, clone_if_doesnt_exist, get_commit_hash, get_commit_metadata
 from util.pydantic import validation_error_str, validation_warning_str
 from util.static import static_url, static_url_path, symbolic_link
@@ -336,14 +336,16 @@ def publish(course_key: str) -> List[str]:
 
     config = None
     errors = []
+    tmpfiles = []
     if Path(store_path).exists():
         with FileLock(store_path):
             try:
                 config = CourseConfig.get(course_key, source=ConfigSource.STORE)
             except ConfigError as e:
                 errors.append(f"Failed to load newly built course for this reason: {e}")
+                logger.warn(f"Failed to load newly built course for this reason: {e}")
             else:
-                renames([
+                tmpfiles = renames([
                     (store_path, prod_path),
                     (store_defaults_path, prod_defaults_path),
                     (store_version_path, prod_version_path),
@@ -356,6 +358,7 @@ def publish(course_key: str) -> List[str]:
                 config = CourseConfig.get(course_key, source=ConfigSource.PUBLISH)
             except ConfigError as e:
                 errors.append(f"Failed to load already published config: {e}")
+                logger.error(f"Failed to load already published config: {e}")
 
     if config is None:
         if errors:
@@ -364,8 +367,13 @@ def publish(course_key: str) -> List[str]:
             raise Exception(f"Course directory not found for {course_key} - the course probably has not been built")
 
     symbolic_link(config)
+    errors = errors + publish_graders(config)
 
-    return errors + publish_graders(config)
+    # Remove temporary files that were created during renaming.
+    # This may block the execution for a while on courses with many files.
+    rm_paths(tmpfiles)
+
+    return errors
 
 
 # the task locks can get stuck if the program suddenly shuts down,

util/files.py

@@ -9,7 +9,7 @@
 import tempfile
 import time
 from types import TracebackType
-from typing import Dict, Generator, Iterable, Optional, Tuple, Type, Union
+from typing import Dict, Generator, Iterable, Optional, Tuple, Type, Union, List
 
 from django.conf import settings
 from django.http.response import FileResponse as DjangoFileResponse, HttpResponse
@@ -46,6 +46,12 @@ def rm_path(path: Union[str, Path]) -> None:
         path.unlink()
 
 
+def rm_paths(paths: Iterable[Union[str, Path]]) -> None:
+    for path in paths:
+        if path is not None:
+            rm_path(path)
+
+
 def is_subpath(child: PathLike, parent: Optional[PathLike] = None) -> bool:
     """
     If parent is not None, returns whether child is a subpath of (contained in)
@@ -168,9 +174,10 @@ def rename(src: PathLike, dst: PathLike, keep_tmp=False) -> Optional[str]:
     return tmpdst
 
 
-def renames(pairs: Iterable[Tuple[PathLike, PathLike]]) -> None:
+def renames(pairs: Iterable[Tuple[PathLike, PathLike]]) -> List[str]:
     """
     Renames multiple files and directories while making sure that either all or none succeed.
+    Returns a list of generated tmp directories (for later removal).
     """
     done = set()
     try:
@@ -183,10 +190,8 @@ def renames(pairs: Iterable[Tuple[PathLike, PathLike]]) -> None:
             if os.path.exists(tmp):
                 rename(tmp, dst)
         raise
-    else:
-        for _, _, tmp in done:
-            if tmp is not None:
-                rm_path(tmp)
+
+    return list(map(lambda x: x[2], done))
 
 
 def _try_lockf(lockfile, flags) -> Optional[OSError]: