From d8ac2a4e21a998036ba26d24aef786dd9ff1a691 Mon Sep 17 00:00:00 2001 From: Alberto Casas Ortiz Date: Mon, 6 Jul 2026 18:27:35 -0700 Subject: [PATCH 1/7] Implemented deletion of S3 files when session or trial is removed. --- mcserver/models.py | 52 +++++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/mcserver/models.py b/mcserver/models.py index e2731e9..e85ebbd 100644 --- a/mcserver/models.py +++ b/mcserver/models.py @@ -1,24 +1,25 @@ -import json - -from django.db import models -from django.contrib.auth.models import AbstractUser -from django.core.validators import MinValueValidator, MaxValueValidator import os import uuid -import base64 -import pathlib from http import HTTPStatus -from django.utils import timezone + +from django.contrib.auth.models import AbstractUser from django.core.exceptions import ValidationError -from django.db.models.signals import post_save -from django.contrib.auth.signals import user_logged_in +from django.db import models +from django.db.models.signals import post_save, pre_delete from django.dispatch import receiver -from rest_framework.authtoken.models import Token +from django.utils import timezone from django.utils.translation import gettext as _ -from rest_framework import status -from django.conf import settings +def delete_s3_file(file_field): + """Delete a file from the configured Django storage backend.""" + if not file_field or not file_field.name: + return + + try: + file_field.delete(save=False) + except Exception as e: + print(f"Error deleting file '{file_field.name}': {e}") def random_filename(instance, filename): return "{}-{}".format(uuid.uuid4(), filename) @@ -255,8 +256,7 @@ class ResetPassword(models.Model): datetime = models.DateField(default=timezone.now) from django_otp.plugins.otp_email.models import EmailDevice -from django.template.loader import render_to_string -from mcserver.customEmailDevice import CustomEmailDevice + @receiver(post_save, sender=User) def create_profile(sender, instance, created, **kwargs): @@ -572,3 +572,25 @@ def get_available_data(self, only_public=False, subject_id=None, share_token=Non subject_ids.append(subject.id) return data + +@receiver(pre_delete, sender=Video) +def delete_video_files(sender, instance, **kwargs): + """Delete Video files when the record is deleted.""" + delete_s3_file(instance.video) + delete_s3_file(instance.video_thumb) + delete_s3_file(instance.keypoints) + +@receiver(pre_delete, sender=Result) +def delete_result_files(sender, instance, **kwargs): + """Delete Result media when the record is deleted.""" + delete_s3_file(instance.media) + +@receiver(pre_delete, sender=Session) +def delete_session_files(sender, instance, **kwargs): + """Delete Session QR code when the record is deleted.""" + delete_s3_file(instance.qrcode) + +@receiver(pre_delete, sender=DownloadLog) +def delete_download_log_files(sender, instance, **kwargs): + """Delete DownloadLog archive when the record is deleted.""" + delete_s3_file(instance.media) \ No newline at end of file From 0cacaf7c2d2292b3e9df0a5931a172731ea2e6ed Mon Sep 17 00:00:00 2001 From: carmichaelong Date: Wed, 22 Jul 2026 16:47:56 -0700 Subject: [PATCH 2/7] fix session_dir storage leak --- mcserver/tasks.py | 48 +++++++------ tests/test_download_cleanup.py | 119 +++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 25 deletions(-) create mode 100644 tests/test_download_cleanup.py diff --git a/mcserver/tasks.py b/mcserver/tasks.py index 6f0391d..3baceff 100644 --- a/mcserver/tasks.py +++ b/mcserver/tasks.py @@ -24,7 +24,8 @@ from mcserver.zipsession_v2 import ( SessionDirectoryConstructor, SubjectDirectoryConstructor, - zipdir + zipdir, + rmtree_with_retry ) @@ -51,57 +52,46 @@ def download_session_archive(self, session_id, user_id=None): """ This task is responsible for asynchronous session archive download. If user_id is None, the public session download occurred. """ - import shutil - - session_dir_path = None + # Known up front so the build dir is cleaned up even if build() raises. + session_dir_path = os.path.join( + settings.MEDIA_ROOT, f"OpenCapData_{session_id}") session_zip_path = None try: session_dir_path = SessionDirectoryConstructor().build(session_id) session_zip_path = zipdir(session_dir_path) - create_download_log(session_zip_path, self.request.id, user_id) - os_remove_with_retry(session_zip_path) - except Exception as e: - # Delete files and send the traceback to Sentry if something went wrong - if session_dir_path and os.path.isfile(session_dir_path): - shutil.rmtree(session_dir_path) - if session_zip_path and os.path.isfile(session_zip_path): - os.remove(session_zip_path) if settings.SENTRY_DSN: import sentry_sdk sentry_sdk.capture_exception(e) else: print(e) + finally: + cleanup_download_tmp_files(session_dir_path, session_zip_path) + @shared_task(bind=True) def download_subject_archive(self, subject_id, user_id): """ This task is responsible for asynchronous subject archive download """ - import shutil - - subject_dir_path = None + # Known up front so the build dir is cleaned up even if build() raises. + subject_dir_path = os.path.join( + settings.MEDIA_ROOT, f"OpenCapData_Subject_{subject_id}") subject_zip_path = None try: subject_dir_path = SubjectDirectoryConstructor().build(subject_id) subject_zip_path = zipdir(subject_dir_path) - with open(subject_zip_path, "rb") as archive: - log = DownloadLog.objects.create(task_id=str(self.request.id), user_id=user_id) - log.media.save(os.path.basename(subject_zip_path), archive) - os.remove(subject_zip_path) + create_download_log(subject_zip_path, self.request.id, user_id) except Exception as e: - # Delete files and send the traceback to Sentry if something went wrong - if subject_dir_path and os.path.isfile(subject_dir_path): - shutil.rmtree(subject_dir_path) - if subject_zip_path and os.path.isfile(subject_zip_path): - os.remove(subject_zip_path) if settings.SENTRY_DSN: import sentry_sdk sentry_sdk.capture_exception(e) else: print(e) + finally: + cleanup_download_tmp_files(subject_dir_path, subject_zip_path) @shared_task @@ -199,7 +189,15 @@ def submit_cloudwatch_metrics(): submit_number_of_pending_trials_to_cloudwatch() # Helper functions -def create_download_log(zip_path, task_id, user_id, +def cleanup_download_tmp_files(dir_path, zip_path): + """Remove the local build dir and zip archive for a download, if present.""" + if dir_path and os.path.isdir(dir_path): + rmtree_with_retry(dir_path) + if zip_path and os.path.isfile(zip_path): + os_remove_with_retry(zip_path) + + +def create_download_log(zip_path, task_id, user_id, max_retries=5, backoff=0.1): archive = None for attempt in range(max_retries): diff --git a/tests/test_download_cleanup.py b/tests/test_download_cleanup.py new file mode 100644 index 0000000..9ca755c --- /dev/null +++ b/tests/test_download_cleanup.py @@ -0,0 +1,119 @@ +import os +import shutil +import tempfile +import zipfile +from unittest import mock + +from django.conf import settings +from django.test import TestCase, override_settings + +from mcserver.models import User, DownloadLog +from mcserver.tasks import download_session_archive, download_subject_archive +from mcserver.zipsession_v2 import ( + SessionDirectoryConstructor, + SubjectDirectoryConstructor, +) + +_TMP = tempfile.mkdtemp() + + +@override_settings( + MEDIA_ROOT=_TMP, + ARCHIVES_ROOT=os.path.join(_TMP, "archives"), + DEFAULT_FILE_STORAGE="django.core.files.storage.FileSystemStorage", + SENTRY_DSN="", +) +class DownloadArchiveCleanupTests(TestCase): + """The download tasks must never leave their build dir / zip on the + worker's ephemeral disk, otherwise it fills up and downloads break.""" + + def setUp(self): + self.user = User.objects.create_user(username="dl-user", password="pw") + os.makedirs(settings.MEDIA_ROOT, exist_ok=True) + + def tearDown(self): + for name in os.listdir(settings.MEDIA_ROOT): + path = os.path.join(settings.MEDIA_ROOT, name) + if os.path.isdir(path): + shutil.rmtree(path, ignore_errors=True) + else: + os.remove(path) + + @staticmethod + def _build_that_creates(dir_path): + """A build() stand-in that actually writes a build dir on disk.""" + def _build(object_id): + os.makedirs(dir_path, exist_ok=True) + with open(os.path.join(dir_path, "payload.txt"), "w") as fh: + fh.write("data") + return dir_path + return _build + + @staticmethod + def _zip_that_succeeds(dir_path): + """A zipdir() stand-in mirroring the real one: remove the source dir + and produce a real zip under ARCHIVES_ROOT.""" + shutil.rmtree(dir_path) + os.makedirs(settings.ARCHIVES_ROOT, exist_ok=True) + zip_path = os.path.join( + settings.ARCHIVES_ROOT, os.path.basename(dir_path) + ".zip") + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("payload.txt", "data") + return zip_path + + # --- session -------------------------------------------------------- + + def test_session_build_dir_removed_when_zip_fails(self): + build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-1") + with mock.patch.object( + SessionDirectoryConstructor, "build", + side_effect=self._build_that_creates(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", + side_effect=OSError("No space left on device"), + ): + download_session_archive.apply(args=("sess-1", self.user.id)) + + self.assertFalse( + os.path.exists(build_dir), + "build dir must be removed after a failed download", + ) + self.assertEqual(DownloadLog.objects.count(), 0) + + def test_session_build_dir_and_zip_removed_on_success(self): + build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-2") + expected_zip = os.path.join( + settings.ARCHIVES_ROOT, "OpenCapData_sess-2.zip") + with mock.patch.object( + SessionDirectoryConstructor, "build", + side_effect=self._build_that_creates(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", side_effect=self._zip_that_succeeds, + ): + download_session_archive.apply(args=("sess-2", self.user.id)) + + self.assertFalse(os.path.exists(build_dir)) + self.assertFalse( + os.path.exists(expected_zip), "local zip must be removed after upload") + self.assertEqual(DownloadLog.objects.count(), 1) + self.assertEqual(DownloadLog.objects.get().user, self.user) + + # --- subject -------------------------------------------------------- + + def test_subject_build_dir_removed_when_zip_fails(self): + build_dir = os.path.join( + settings.MEDIA_ROOT, "OpenCapData_Subject_subj-1") + with mock.patch.object( + SubjectDirectoryConstructor, "build", + side_effect=self._build_that_creates(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", + side_effect=OSError("No space left on device"), + ): + download_subject_archive.apply(args=("subj-1", self.user.id)) + + self.assertFalse( + os.path.exists(build_dir), + "build dir must be removed after a failed download", + ) + self.assertEqual(DownloadLog.objects.count(), 0) From e5b72daeccde92a712497d640156fad39b889f06 Mon Sep 17 00:00:00 2001 From: carmichaelong Date: Thu, 23 Jul 2026 11:46:16 -0700 Subject: [PATCH 3/7] fold download/cleanup tests into test_tasks. replace some stale tests with these --- tests/test_download_cleanup.py | 119 ------------------ tests/test_tasks.py | 214 +++++++++++++++++++++++---------- 2 files changed, 149 insertions(+), 184 deletions(-) delete mode 100644 tests/test_download_cleanup.py diff --git a/tests/test_download_cleanup.py b/tests/test_download_cleanup.py deleted file mode 100644 index 9ca755c..0000000 --- a/tests/test_download_cleanup.py +++ /dev/null @@ -1,119 +0,0 @@ -import os -import shutil -import tempfile -import zipfile -from unittest import mock - -from django.conf import settings -from django.test import TestCase, override_settings - -from mcserver.models import User, DownloadLog -from mcserver.tasks import download_session_archive, download_subject_archive -from mcserver.zipsession_v2 import ( - SessionDirectoryConstructor, - SubjectDirectoryConstructor, -) - -_TMP = tempfile.mkdtemp() - - -@override_settings( - MEDIA_ROOT=_TMP, - ARCHIVES_ROOT=os.path.join(_TMP, "archives"), - DEFAULT_FILE_STORAGE="django.core.files.storage.FileSystemStorage", - SENTRY_DSN="", -) -class DownloadArchiveCleanupTests(TestCase): - """The download tasks must never leave their build dir / zip on the - worker's ephemeral disk, otherwise it fills up and downloads break.""" - - def setUp(self): - self.user = User.objects.create_user(username="dl-user", password="pw") - os.makedirs(settings.MEDIA_ROOT, exist_ok=True) - - def tearDown(self): - for name in os.listdir(settings.MEDIA_ROOT): - path = os.path.join(settings.MEDIA_ROOT, name) - if os.path.isdir(path): - shutil.rmtree(path, ignore_errors=True) - else: - os.remove(path) - - @staticmethod - def _build_that_creates(dir_path): - """A build() stand-in that actually writes a build dir on disk.""" - def _build(object_id): - os.makedirs(dir_path, exist_ok=True) - with open(os.path.join(dir_path, "payload.txt"), "w") as fh: - fh.write("data") - return dir_path - return _build - - @staticmethod - def _zip_that_succeeds(dir_path): - """A zipdir() stand-in mirroring the real one: remove the source dir - and produce a real zip under ARCHIVES_ROOT.""" - shutil.rmtree(dir_path) - os.makedirs(settings.ARCHIVES_ROOT, exist_ok=True) - zip_path = os.path.join( - settings.ARCHIVES_ROOT, os.path.basename(dir_path) + ".zip") - with zipfile.ZipFile(zip_path, "w") as zf: - zf.writestr("payload.txt", "data") - return zip_path - - # --- session -------------------------------------------------------- - - def test_session_build_dir_removed_when_zip_fails(self): - build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-1") - with mock.patch.object( - SessionDirectoryConstructor, "build", - side_effect=self._build_that_creates(build_dir), - ), mock.patch( - "mcserver.tasks.zipdir", - side_effect=OSError("No space left on device"), - ): - download_session_archive.apply(args=("sess-1", self.user.id)) - - self.assertFalse( - os.path.exists(build_dir), - "build dir must be removed after a failed download", - ) - self.assertEqual(DownloadLog.objects.count(), 0) - - def test_session_build_dir_and_zip_removed_on_success(self): - build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-2") - expected_zip = os.path.join( - settings.ARCHIVES_ROOT, "OpenCapData_sess-2.zip") - with mock.patch.object( - SessionDirectoryConstructor, "build", - side_effect=self._build_that_creates(build_dir), - ), mock.patch( - "mcserver.tasks.zipdir", side_effect=self._zip_that_succeeds, - ): - download_session_archive.apply(args=("sess-2", self.user.id)) - - self.assertFalse(os.path.exists(build_dir)) - self.assertFalse( - os.path.exists(expected_zip), "local zip must be removed after upload") - self.assertEqual(DownloadLog.objects.count(), 1) - self.assertEqual(DownloadLog.objects.get().user, self.user) - - # --- subject -------------------------------------------------------- - - def test_subject_build_dir_removed_when_zip_fails(self): - build_dir = os.path.join( - settings.MEDIA_ROOT, "OpenCapData_Subject_subj-1") - with mock.patch.object( - SubjectDirectoryConstructor, "build", - side_effect=self._build_that_creates(build_dir), - ), mock.patch( - "mcserver.tasks.zipdir", - side_effect=OSError("No space left on device"), - ): - download_subject_archive.apply(args=("subj-1", self.user.id)) - - self.assertFalse( - os.path.exists(build_dir), - "build dir must be removed after a failed download", - ) - self.assertEqual(DownloadLog.objects.count(), 0) diff --git a/tests/test_tasks.py b/tests/test_tasks.py index 1ebabb1..b08212e 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -1,8 +1,11 @@ import os import json +import shutil import tempfile +import zipfile from unittest import mock +from django.conf import settings from django.test import TestCase, override_settings from mcserver.models import ( @@ -27,6 +30,7 @@ ) _temp_media = tempfile.mkdtemp() +_download_tmp = tempfile.mkdtemp() # isolated dir for DownloadArchiveTests' tearDown @override_settings( @@ -54,66 +58,6 @@ def setUp(self): self.trial_one = Trial.objects.create(session=self.session, name="testone") self.trial_two = Trial.objects.create(session=self.session, name="testtwo") - @mock.patch("mcserver.tasks.zipdir") - @mock.patch.object(SessionDirectoryConstructor, "build") - def test_download_session_archive_creates_archive_and_logs_action( - self, mock_dir_builder, mock_zipdir - ): - mock_zipdir.return_value = "archive.zip" - mock_dir_builder.return_value = "archive" - before_logs = DownloadLog.objects.count() - task = download_session_archive.delay("dummy-session-id", self.user.id) - after_logs = DownloadLog.objects.count() - self.assertEqual(after_logs, before_logs + 1) - - log = DownloadLog.objects.last() - self.assertEqual(log.task_id, task.id) - self.assertEqual(log.user, self.user) - self.assertEqual(log.media_path, "archive.zip") - - mock_dir_builder.assert_called_once_with("dummy-session-id") - mock_zipdir.assert_called_once_with("archive") - - @mock.patch("mcserver.tasks.zipdir") - @mock.patch.object(SessionDirectoryConstructor, "build") - def test_download_session_archive_creates_archive_and_logs_action_for_anon_user( - self, mock_dir_builder, mock_zipdir - ): - mock_zipdir.return_value = "archive.zip" - mock_dir_builder.return_value = "archive" - before_logs = DownloadLog.objects.count() - task = download_session_archive.delay("dummy-session-id", None) - after_logs = DownloadLog.objects.count() - self.assertEqual(after_logs, before_logs + 1) - - log = DownloadLog.objects.last() - self.assertEqual(log.task_id, task.id) - self.assertIsNone(log.user) - self.assertEqual(log.media_path, "archive.zip") - - mock_dir_builder.assert_called_once_with("dummy-session-id") - mock_zipdir.assert_called_once_with("archive") - - @mock.patch("mcserver.tasks.zipdir") - @mock.patch.object(SubjectDirectoryConstructor, "build") - def test_download_subject_archive_creates_archive_and_logs_action( - self, mock_dir_builder, mock_zipdir - ): - mock_zipdir.return_value = "archive.zip" - mock_dir_builder.return_value = "archive" - before_logs = DownloadLog.objects.count() - task = download_subject_archive.delay("dummy-subject-id", self.user.id) - after_logs = DownloadLog.objects.count() - self.assertEqual(after_logs, before_logs + 1) - - log = DownloadLog.objects.last() - self.assertEqual(log.task_id, task.id) - self.assertEqual(log.user, self.user) - self.assertEqual(log.media_path, "archive.zip") - - mock_dir_builder.assert_called_once_with("dummy-subject-id") - mock_zipdir.assert_called_once_with("archive") - def test_delete_pingdom_sessions_successful(self): Session.objects.create(user=self.pingdom_user) Session.objects.create(user=self.pingdom_user) @@ -137,7 +81,7 @@ def test_delete_pingdom_sessions_no_sessions(self): self.assertFalse(Session.objects.filter(user=self.pingdom_user).exists()) delete_pingdom_sessions.delay() self.assertFalse(Session.objects.filter(user=self.pingdom_user).exists()) - + @mock.patch("requests.post") def test_invoke_aws_lambda_function_commits_successful_analysis_result( self, mock_post_request @@ -201,7 +145,7 @@ def test_invoke_aws_lambda_function_commits_failed_analysis_result_if_aws_error( self.assertIsNone(analysis_result.result) self.assertEqual(analysis_result.response, {'error': 'session_id is required.'}) self.assertEqual(analysis_result.state, AnalysisResultState.FAILED) - + def test_invoke_aws_lambda_function_commits_failed_analysis_result_if_request_exception( self ): @@ -225,7 +169,7 @@ def test_invoke_aws_lambda_function_commits_failed_analysis_result_if_request_ex ) self.assertIsNone(analysis_result.result) self.assertEqual(analysis_result.state, AnalysisResultState.FAILED) - + @mock.patch("requests.post") def test_invoke_aws_lambda_function_commits_failed_analysis_result_if_json_invalid( self, mock_post_request @@ -252,7 +196,7 @@ def test_invoke_aws_lambda_function_commits_failed_analysis_result_if_json_inval self.assertIsNone(analysis_result.result) self.assertEqual(analysis_result.response, {'error': 'Invalid JSON.'}) self.assertEqual(analysis_result.state, AnalysisResultState.FAILED) - + @mock.patch("requests.post") def test_invoke_aws_lambda_function_re_run_analysis_function_in_the_same_result_instance( self, mock_post_request @@ -285,4 +229,144 @@ def test_invoke_aws_lambda_function_re_run_analysis_function_in_the_same_result_ self.assertEqual(analisys_result.data, data) self.assertEqual(analisys_result.status, status_code) self.assertEqual(analisys_result.result, result) - self.assertEqual(analisys_result.state, AnalysisResultState.SUCCESSFULL) \ No newline at end of file + self.assertEqual(analisys_result.state, AnalysisResultState.SUCCESSFULL) + + +@override_settings( + MEDIA_ROOT=_download_tmp, + ARCHIVES_ROOT=os.path.join(_download_tmp, "archives"), + DEFAULT_FILE_STORAGE="django.core.files.storage.FileSystemStorage", + SENTRY_DSN="", +) +class DownloadArchiveTests(TestCase): + """download_session_archive / download_subject_archive must create a + DownloadLog on success and never leave the build dir or zip on the + worker's ephemeral disk (which otherwise fills up and breaks downloads). + """ + + def setUp(self): + self.user = User.objects.create_user(username="dl-user", password="pw") + os.makedirs(settings.MEDIA_ROOT, exist_ok=True) + + def tearDown(self): + for name in os.listdir(settings.MEDIA_ROOT): + path = os.path.join(settings.MEDIA_ROOT, name) + if os.path.isdir(path): + shutil.rmtree(path, ignore_errors=True) + else: + os.remove(path) + + @staticmethod + def _make_fake_build(dir_path): + """Return a fake build() (Session/SubjectDirectoryConstructor.build) + that writes a real build dir on disk and returns its path.""" + def _build(object_id): + os.makedirs(dir_path, exist_ok=True) + with open(os.path.join(dir_path, "payload.txt"), "w") as fh: + fh.write("data") + return dir_path + return _build + + @staticmethod + def _fake_zipdir(dir_path): + """Fake zipdir() for the success path, mirroring the real one: remove + the source dir and write a real zip under ARCHIVES_ROOT.""" + shutil.rmtree(dir_path) + os.makedirs(settings.ARCHIVES_ROOT, exist_ok=True) + zip_path = os.path.join( + settings.ARCHIVES_ROOT, os.path.basename(dir_path) + ".zip") + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("payload.txt", "data") + return zip_path + + # --- success: DownloadLog created and temp files cleaned up --------- + + def test_session_success_creates_log_and_cleans_up(self): + build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-1") + leftover_zip = os.path.join( + settings.ARCHIVES_ROOT, "OpenCapData_sess-1.zip") + with mock.patch.object( + SessionDirectoryConstructor, "build", + side_effect=self._make_fake_build(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", side_effect=self._fake_zipdir, + ): + result = download_session_archive.apply(args=("sess-1", self.user.id)) + + self.assertEqual(DownloadLog.objects.count(), 1) + log = DownloadLog.objects.get() + self.assertEqual(log.user, self.user) + self.assertEqual(log.task_id, result.id) + self.assertTrue(log.media.name) # a file was actually stored + self.assertFalse(os.path.exists(build_dir)) + self.assertFalse(os.path.exists(leftover_zip)) + + def test_session_success_for_anonymous_user(self): + build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-anon") + with mock.patch.object( + SessionDirectoryConstructor, "build", + side_effect=self._make_fake_build(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", side_effect=self._fake_zipdir, + ): + download_session_archive.apply(args=("sess-anon", None)) + + self.assertEqual(DownloadLog.objects.count(), 1) + self.assertIsNone(DownloadLog.objects.get().user) + + def test_subject_success_creates_log_and_cleans_up(self): + build_dir = os.path.join( + settings.MEDIA_ROOT, "OpenCapData_Subject_subj-1") + leftover_zip = os.path.join( + settings.ARCHIVES_ROOT, "OpenCapData_Subject_subj-1.zip") + with mock.patch.object( + SubjectDirectoryConstructor, "build", + side_effect=self._make_fake_build(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", side_effect=self._fake_zipdir, + ): + result = download_subject_archive.apply(args=("subj-1", self.user.id)) + + self.assertEqual(DownloadLog.objects.count(), 1) + log = DownloadLog.objects.get() + self.assertEqual(log.user, self.user) + self.assertEqual(log.task_id, result.id) + self.assertFalse(os.path.exists(build_dir)) + self.assertFalse(os.path.exists(leftover_zip)) + + # --- failure: no DownloadLog, but temp still cleaned up ------------ + + def test_session_cleans_up_build_dir_when_zip_fails(self): + build_dir = os.path.join(settings.MEDIA_ROOT, "OpenCapData_sess-2") + with mock.patch.object( + SessionDirectoryConstructor, "build", + side_effect=self._make_fake_build(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", + side_effect=OSError("No space left on device"), + ): + download_session_archive.apply(args=("sess-2", self.user.id)) + + self.assertFalse( + os.path.exists(build_dir), + "build dir must be removed after a failed download", + ) + self.assertEqual(DownloadLog.objects.count(), 0) + + def test_subject_cleans_up_build_dir_when_zip_fails(self): + build_dir = os.path.join( + settings.MEDIA_ROOT, "OpenCapData_Subject_subj-2") + with mock.patch.object( + SubjectDirectoryConstructor, "build", + side_effect=self._make_fake_build(build_dir), + ), mock.patch( + "mcserver.tasks.zipdir", + side_effect=OSError("No space left on device"), + ): + download_subject_archive.apply(args=("subj-2", self.user.id)) + + self.assertFalse( + os.path.exists(build_dir), + "build dir must be removed after a failed download", + ) + self.assertEqual(DownloadLog.objects.count(), 0) From f3da886247937beb4d12716b1dd40ca468341325 Mon Sep 17 00:00:00 2001 From: Alberto Casas Ortiz Date: Tue, 28 Jul 2026 12:13:41 -0700 Subject: [PATCH 4/7] Updated README.md --- README.md | 196 ++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 167 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 40b38d5..e1c3a18 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,31 @@ -# OpenCap API -## Workflow for app.opencap.ai +# ๐ŸŽฏ OpenCap API + +[![Python Version](https://img.shields.io/badge/python-3.7+-blue.svg)](https://www.python.org/downloads/) +[![Django Version](https://img.shields.io/badge/django-3.1.14+-green.svg)](https://www.djangoproject.com/) +[![License](https://img.shields.io/badge/license-APACHE_2.0-blue.svg)](LICENSE.md) +[![PRs Welcome](https://img.shields.io/badge/PRs-welcome-brightgreen.svg)](https://github.com/opencap-org/opencap-api/pulls) + +> Backend API for OpenCap - The open-source platform for biomechanical motion capture and gait analysis using mobile devices. + +## ๐Ÿ“– Table of Contents + +- [Overview](#overview) +- [Workflow](#workflow) +- [Getting Started](#getting-started) +- [API Documentation](#api-documentation) +- [Development](#development) +- [Testing](#testing) +- [Internationalization](#internationalization) +- [Deployment](#deployment) +- [Contributing](#contributing) +- [License](#license) + +## ๐Ÿ”ญ Overview + +OpenCap is an open source biomechanical motion capture platform that leverages iOS devices to capture and analyze human movement. This repository contains the Django-based backend API that orchestrates the entire workflow, from session management to video processing and biomechanical analysis. + +## ๐Ÿ”„ Workflow + 1. User enters the website (app.opencap.ai) 2. The website calls the backend and creates a session 3. The session generates a QR code displayed in the webapp @@ -12,56 +38,168 @@ 10. Video processing pipeline pools sessions in 'processing' state and processes them 11. After processing, results are sent to the backend and the backend changes its state to 'done' -## Installation +## ๐Ÿš€ Getting Started + +### Prerequisites + +- Python 3.7+ +- [gettext](https://www.gnu.org/software/gettext/) (for internationalization) + +### Installation -Clone this repo, then: +1. Clone the repository: +```bash +git clone https://github.com/opencap-org/opencap-api.git +cd opencap-api ``` -conda create -n opencap python=3.7 + +2. Create and activate a conda environment: +```bash +conda create -n opencap python=3.7 conda activate opencap -pip install -r requirements.txt ``` -Create the `.env` file with all env variables and credentials -## Running the server locally +3. Install dependencies: +```bash +pip install -r requirements.txt +``` +4. Create environment variables file: +```bash +touch .env +# Edit .env with your credentials ``` + +5. Start the development server: +```bash python manage.py runserver ``` -## Adding new fields to the data model +The API will be available at `http://localhost:8000/` by default. + +## ๐Ÿ“š API Documentation + +### Interactive Docs + +Once the server is running, access the auto-generated documentation: + +- **Swagger UI**: `http://localhost:8000/docs/` +- **ReDoc**: `http://localhost:8000/redocs/` + +## ๐Ÿ’ป Development + +### Adding New Fields to the Data Model + +1. Update models in `mcserver/models.py`: +```python +class YourModel(models.Model): + new_field = models.CharField(max_length=255) +``` + +2. Create migration: +```bash +python manage.py makemigrations +``` + +3. Apply migration: +```bash +python manage.py migrate # Careful: modifies database! +``` + +4. Update serializers in `mcserver/serializers.py`: +```python +class YourModelSerializer(serializers.ModelSerializer): + class Meta: + fields = [... 'new_field'] +``` + +5. Potentially update `mcserver/admin.py` + +### Running Tests + +```bash +python manage.py test ./tests/ +``` -1. Add fields to `mcserver/models.py` -2. Run `python manage.py makemigrations` -3. Run `python manage.py migrate` (be careful, this modifies the database) -4. Add fields we want to expose in the api to the `mcserver/serializers.py` file +> **Note**: Some tests may be outdated and fail. Test `test_permissions.SessionsPermissionsTests` may fail on Windows but works on Ubuntu and macOS. -Then for deploying to production we pull all the updated code and run the step 3. (with the production `.env` file) +## ๐ŸŒ Internationalization -## Internationalization/Localization +### Adding New Languages -Instructions in this [Link](https://docs.djangoproject.com/en/4.2/topics/i18n/translation/). +Navigate to the `mcserver` folder: -**Note:** You must also install [gettext](https://www.gnu.org/software/gettext/). After install, restart your IDE/Terminal). +1. Create translation files for a language: +```bash +django-admin makemessages -l +# Example: django-admin makemessages -l es +``` + +2. Compile translation messages: +```bash +django-admin compilemessages +``` + +> **Note**: Make sure gettext is installed and your IDE/Terminal is restarted after installation. -Inside of mcserver folder: +## ๐Ÿšข Deployment -1. Create files for a language: +### Production Deployment Steps - `django-admin makemessages -l ` +1. Pull the latest code: +```bash +git pull origin main +``` + +2. Update dependencies: +```bash +pip install -r requirements.txt +``` + +3. Run migrations: +```bash +python manage.py migrate +``` + +4. Restart the application server (Gunicorn/uWSGI/etc.) + +## ๐Ÿงช Testing + +### Test Coverage + +Run tests with coverage: + +```bash +coverage run manage.py test ./tests/ +coverage report -m +``` + +### API Testing + +Use the Swagger UI at `/docs/` or use tools like curl: + +```bash +# Create a session +curl -X POST http://localhost:8000/sessions/ \ + -H "Authorization: Token your_token" \ + -H "Content-Type: application/json" \ + -d '{"subject": "subject_uuid"}' +``` -2. Compile messages: +## ๐Ÿค Contributing - `django-admin compilemessages` +We welcome contributions! Please submit an [Issue](https://github.com/opencap-org/opencap-api/issues) or create a [PR](https://github.com/opencap-org/opencap-api/pulls). +### Development Workflow -## Current routes (not up to date, there are more): +1. Fork the repository +2. Create a feature branch (`git checkout -b feature/AmazingFeature`) +3. Commit your changes (`git commit -m 'Add some AmazingFeature'`) +4. Push to the branch (`git push origin feature/AmazingFeature`) +5. Open a Pull Request -/sessions/new/ -> returns session_id and the QR code -/sessions//status/?device_id= <- devices use this link to register and get video_id +## ๐Ÿ“„ License -/sessions//record/ -> server uses this link to start recording +This project is licensed under the Apache License 2.0 - see the [LICENSE](LICENSE.md) file for details. -/sessions//stop/ -> server uses this link to stop recording - -/video// <- devices use this link to upload the recorded video and parameters From 25da8c6333aa1236fdd0e7acf8f5ef5042f08fa5 Mon Sep 17 00:00:00 2001 From: Alberto Casas Ortiz Date: Tue, 28 Jul 2026 12:17:02 -0700 Subject: [PATCH 5/7] Reverted S3 changes in models --- mcserver/models.py | 52 +++++++++++++--------------------------------- 1 file changed, 15 insertions(+), 37 deletions(-) diff --git a/mcserver/models.py b/mcserver/models.py index e85ebbd..e2731e9 100644 --- a/mcserver/models.py +++ b/mcserver/models.py @@ -1,25 +1,24 @@ +import json + +from django.db import models +from django.contrib.auth.models import AbstractUser +from django.core.validators import MinValueValidator, MaxValueValidator import os import uuid +import base64 +import pathlib from http import HTTPStatus - -from django.contrib.auth.models import AbstractUser +from django.utils import timezone from django.core.exceptions import ValidationError -from django.db import models -from django.db.models.signals import post_save, pre_delete +from django.db.models.signals import post_save +from django.contrib.auth.signals import user_logged_in from django.dispatch import receiver -from django.utils import timezone +from rest_framework.authtoken.models import Token from django.utils.translation import gettext as _ +from rest_framework import status +from django.conf import settings -def delete_s3_file(file_field): - """Delete a file from the configured Django storage backend.""" - if not file_field or not file_field.name: - return - - try: - file_field.delete(save=False) - except Exception as e: - print(f"Error deleting file '{file_field.name}': {e}") def random_filename(instance, filename): return "{}-{}".format(uuid.uuid4(), filename) @@ -256,7 +255,8 @@ class ResetPassword(models.Model): datetime = models.DateField(default=timezone.now) from django_otp.plugins.otp_email.models import EmailDevice - +from django.template.loader import render_to_string +from mcserver.customEmailDevice import CustomEmailDevice @receiver(post_save, sender=User) def create_profile(sender, instance, created, **kwargs): @@ -572,25 +572,3 @@ def get_available_data(self, only_public=False, subject_id=None, share_token=Non subject_ids.append(subject.id) return data - -@receiver(pre_delete, sender=Video) -def delete_video_files(sender, instance, **kwargs): - """Delete Video files when the record is deleted.""" - delete_s3_file(instance.video) - delete_s3_file(instance.video_thumb) - delete_s3_file(instance.keypoints) - -@receiver(pre_delete, sender=Result) -def delete_result_files(sender, instance, **kwargs): - """Delete Result media when the record is deleted.""" - delete_s3_file(instance.media) - -@receiver(pre_delete, sender=Session) -def delete_session_files(sender, instance, **kwargs): - """Delete Session QR code when the record is deleted.""" - delete_s3_file(instance.qrcode) - -@receiver(pre_delete, sender=DownloadLog) -def delete_download_log_files(sender, instance, **kwargs): - """Delete DownloadLog archive when the record is deleted.""" - delete_s3_file(instance.media) \ No newline at end of file From 0a45817b3ce15f3bba15f5f05b4ba7a966e1d6ac Mon Sep 17 00:00:00 2001 From: Alberto Casas Ortiz Date: Tue, 28 Jul 2026 12:19:03 -0700 Subject: [PATCH 6/7] Removed coverage, as not integrated yet. --- README.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index e1c3a18..d960c37 100644 --- a/README.md +++ b/README.md @@ -165,13 +165,12 @@ python manage.py migrate ## ๐Ÿงช Testing -### Test Coverage +### Test -Run tests with coverage: +Run tests with: ```bash -coverage run manage.py test ./tests/ -coverage report -m +run manage.py test ./tests/ ``` ### API Testing From bcf8e9835cb595f7bd02fd0c36063b4c86ccbca4 Mon Sep 17 00:00:00 2001 From: carmichaelong Date: Fri, 31 Jul 2026 15:54:31 -0700 Subject: [PATCH 7/7] clean up README --- README.md | 182 +++++++++++++++++------------------------------------- 1 file changed, 57 insertions(+), 125 deletions(-) diff --git a/README.md b/README.md index d960c37..c837daa 100644 --- a/README.md +++ b/README.md @@ -1,28 +1,14 @@ # ๐ŸŽฏ OpenCap API -[![Python Version](https://img.shields.io/badge/python-3.7+-blue.svg)](https://www.python.org/downloads/) -[![Django Version](https://img.shields.io/badge/django-3.1.14+-green.svg)](https://www.djangoproject.com/) +[![Python Version](https://img.shields.io/badge/python-3.7-blue.svg)](https://www.python.org/downloads/) +[![Django Version](https://img.shields.io/badge/django-3.1.14-green.svg)](https://www.djangoproject.com/) [![License](https://img.shields.io/badge/license-APACHE_2.0-blue.svg)](LICENSE.md) -[![PRs Welcome](https://img.shields.io/badge/PRs-welcome-brightgreen.svg)](https://github.com/opencap-org/opencap-api/pulls) -> Backend API for OpenCap - The open-source platform for biomechanical motion capture and gait analysis using mobile devices. - -## ๐Ÿ“– Table of Contents - -- [Overview](#overview) -- [Workflow](#workflow) -- [Getting Started](#getting-started) -- [API Documentation](#api-documentation) -- [Development](#development) -- [Testing](#testing) -- [Internationalization](#internationalization) -- [Deployment](#deployment) -- [Contributing](#contributing) -- [License](#license) +> Django backend for [OpenCap](https://app.opencap.ai). ## ๐Ÿ”ญ Overview -OpenCap is an open source biomechanical motion capture platform that leverages iOS devices to capture and analyze human movement. This repository contains the Django-based backend API that orchestrates the entire workflow, from session management to video processing and biomechanical analysis. +The API serves both the webapp and the iOS app. It stores sessions, subjects, trials, and videos, and queues recordings for the processing pipeline. Background work (archive builds, session downloads, scheduled cleanup) runs in Celery workers backed by Redis, not in the web process. ## ๐Ÿ”„ Workflow @@ -42,163 +28,109 @@ OpenCap is an open source biomechanical motion capture platform that leverages i ### Prerequisites -- Python 3.7+ -- [gettext](https://www.gnu.org/software/gettext/) (for internationalization) +Beyond `requirements.txt`, which covers pip packages only: + +- **Python 3.7** โ€” the pinned dependencies do not install on newer versions +- **PostgreSQL** โ€” `mcserver/settings.py` always uses the `postgresql` backend + +Depending on the work: + +- **Redis** โ€” to run Celery. The API serves requests without it, but asynchronous work (session downloads, cleanup jobs) never runs +- **[gettext](https://www.gnu.org/software/gettext/)** โ€” to compile translations ### Installation -1. Clone the repository: ```bash git clone https://github.com/opencap-org/opencap-api.git cd opencap-api -``` - -2. Create and activate a conda environment: -```bash conda create -n opencap python=3.7 conda activate opencap -``` - -3. Install dependencies: -```bash pip install -r requirements.txt ``` -4. Create environment variables file: -```bash -touch .env -# Edit .env with your credentials -``` +### Configuration + +Create a `.env` file in the repository root. The required variables are the `config(...)` calls in `mcserver/settings.py` that have no default; ask a maintainer for development values. + +### Running locally -5. Start the development server: ```bash +python manage.py migrate python manage.py runserver ``` -The API will be available at `http://localhost:8000/` by default. +The API is served at `http://localhost:8000/`. -## ๐Ÿ“š API Documentation +Asynchronous work needs Celery processes running alongside the server: -### Interactive Docs +```bash +celery -A mcserver worker -l info # session downloads, archive builds +celery -A mcserver beat -l info # scheduled jobs +``` -Once the server is running, access the auto-generated documentation: +## ๐Ÿ“š API Documentation + +With the server running: - **Swagger UI**: `http://localhost:8000/docs/` - **ReDoc**: `http://localhost:8000/redocs/` -## ๐Ÿ’ป Development - -### Adding New Fields to the Data Model - -1. Update models in `mcserver/models.py`: -```python -class YourModel(models.Model): - new_field = models.CharField(max_length=255) -``` +Routes are registered in `mcserver/urls.py`. Most come from the viewsets in `mcserver/views.py`, which also add a number of custom actions on top of the standard REST routes. Requests authenticate with a DRF token, or a session cookie for the browsable API. -2. Create migration: -```bash -python manage.py makemigrations -``` +## ๐Ÿ’ป Development -3. Apply migration: -```bash -python manage.py migrate # Careful: modifies database! -``` +### Adding new fields to the data model -4. Update serializers in `mcserver/serializers.py`: -```python -class YourModelSerializer(serializers.ModelSerializer): - class Meta: - fields = [... 'new_field'] -``` +1. Add the field to the model in `mcserver/models.py` +2. Run `python manage.py makemigrations` +3. Run `python manage.py migrate` โ€” careful, this modifies the database +4. Add the field to `mcserver/serializers.py` if the API should expose it +5. Update `mcserver/admin.py` if it should appear in the admin -5. Potentially update `mcserver/admin.py` +### Running tests -### Running Tests +Tests need a valid `.env` and a database they are allowed to create. ```bash -python manage.py test ./tests/ +python manage.py test tests # whole suite +python manage.py test tests.test_permissions # one module ``` > **Note**: Some tests may be outdated and fail. Test `test_permissions.SessionsPermissionsTests` may fail on Windows but works on Ubuntu and macOS. ## ๐ŸŒ Internationalization -### Adding New Languages +See the [Django translation docs](https://docs.djangoproject.com/en/3.1/topics/i18n/translation/). This requires [gettext](https://www.gnu.org/software/gettext/) โ€” restart your terminal or IDE after installing it. -Navigate to the `mcserver` folder: - -1. Create translation files for a language: -```bash -django-admin makemessages -l -# Example: django-admin makemessages -l es -``` +From the `mcserver` folder: -2. Compile translation messages: ```bash -django-admin compilemessages +django-admin makemessages -l es # create or refresh files for a language +django-admin compilemessages # compile them ``` -> **Note**: Make sure gettext is installed and your IDE/Terminal is restarted after installation. - ## ๐Ÿšข Deployment -### Production Deployment Steps +Deployment is automated with GitHub Actions. Each push builds `Dockerfile`, pushes the image to ECR, and forces a new ECS deployment. -1. Pull the latest code: -```bash -git pull origin main -``` +| Branch | Workflow | Effect | +| --- | --- | --- | +| `dev` | `.github/workflows/ecr-dev.yml` | Builds `opencap/api-dev`; redeploys `api-server-dev`, `api-server-celery-dev`, `api-server-celery-beat-dev` in `opencap-api-cluster-dev` | +| `main` | `.github/workflows/ecr.yml` | Builds `opencap/api`; redeploys `api-server`, `api-server-celery`, `api-server-celery-beat` in `opencap-api-cluster` | -2. Update dependencies: -```bash -pip install -r requirements.txt -``` - -3. Run migrations: -```bash -python manage.py migrate -``` +Two things are not automated: -4. Restart the application server (Gunicorn/uWSGI/etc.) - -## ๐Ÿงช Testing - -### Test - -Run tests with: - -```bash -run manage.py test ./tests/ -``` - -### API Testing - -Use the Swagger UI at `/docs/` or use tools like curl: - -```bash -# Create a session -curl -X POST http://localhost:8000/sessions/ \ - -H "Authorization: Token your_token" \ - -H "Content-Type: application/json" \ - -d '{"subject": "subject_uuid"}' -``` +- **Migrations.** If your change adds one, run `python manage.py migrate` against that environment's database after the deploy. +- **Environment variables.** New settings have to be added to the ECS task definitions; they are not read from this repository. ## ๐Ÿค Contributing -We welcome contributions! Please submit an [Issue](https://github.com/opencap-org/opencap-api/issues) or create a [PR](https://github.com/opencap-org/opencap-api/pulls). - -### Development Workflow - -1. Fork the repository -2. Create a feature branch (`git checkout -b feature/AmazingFeature`) -3. Commit your changes (`git commit -m 'Add some AmazingFeature'`) -4. Push to the branch (`git push origin feature/AmazingFeature`) -5. Open a Pull Request +1. Open an [issue](https://github.com/opencap-org/opencap-api/issues) describing the change first +2. Branch off `dev` +3. Open a pull request against `dev`, referencing the issue +`dev` is the integration branch. Changes reach production through a `dev` โ†’ `main` pull request. Since a push to `main` deploys production immediately, work should not target it directly. ## ๐Ÿ“„ License -This project is licensed under the Apache License 2.0 - see the [LICENSE](LICENSE.md) file for details. - +Apache License 2.0 โ€” see [LICENSE.md](LICENSE.md) for details.