diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 08fc022c..a78abdbc 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -59,6 +59,7 @@ Fixed - **Landingzones** - Stats badge displayed to superusers for ``DELETED`` zones (#1866) - Zone status updating not working in project details card (#1902) + - Modifying finished lock status allowed in ``SetLandingZoneStatusTask`` (#1909) - **Samplesheets** - Invalid WebDAV URLs generated in ``IrodsDataRequestListView`` (#1860) - Superuser not allowed to edit iRODS request from other users in UI (#1863) @@ -71,6 +72,7 @@ Fixed - Hardcoded iRODS path length in ``landing_zone_move`` (#1888) - Uncaught exceptions in ``SetAccessTask`` (#1906) - Crash in ``landing_zone_create`` with large amount of collections (#1905) + - Finished landing zone status modified by lock exception (#1909) Removed ------- diff --git a/landingzones/tasks_taskflow.py b/landingzones/tasks_taskflow.py index 71911796..875fee5b 100644 --- a/landingzones/tasks_taskflow.py +++ b/landingzones/tasks_taskflow.py @@ -19,6 +19,7 @@ from landingzones.constants import ( STATUS_BUSY, + STATUS_FINISHED, ZONE_STATUS_FAILED, ZONE_STATUS_NOT_CREATED, ZONE_STATUS_MOVED, @@ -381,10 +382,13 @@ def execute( *args, **kwargs ): - self.set_status( - landing_zone, flow_name, status, status_info, extra_data - ) - self.data_modified = True + # Prevent setting status if already finished (see #1909) + landing_zone.refresh_from_db() + if landing_zone.status not in STATUS_FINISHED: + self.set_status( + landing_zone, flow_name, status, status_info, extra_data + ) + self.data_modified = True super().execute(*args, **kwargs) def revert( diff --git a/taskflowbackend/api.py b/taskflowbackend/api.py index 708ff52a..60a47d36 100644 --- a/taskflowbackend/api.py +++ b/taskflowbackend/api.py @@ -8,6 +8,7 @@ ZONE_STATUS_NOT_CREATED, ZONE_STATUS_CREATING, ZONE_STATUS_FAILED, + STATUS_FINISHED, ) from landingzones.models import LandingZone @@ -62,6 +63,29 @@ def _raise_flow_exception(cls, ex_msg, tl_event=None, zone=None): # TODO: Create app alert for failure if async (see #1499) raise cls.FlowSubmitException(ex_msg) + @classmethod + def _raise_lock_exception(cls, ex_msg, tl_event=None, zone=None): + """ + Raise exception specifically for project lock errors. Updates the status + of the landing zone only if zone has not yet been finished by a + concurrent taskflow. + + :param ex_msg: Exception message (string) + :param tl_event: Timeline event or None + :param zone: LandingZone object or None + :raise: FlowSubmitException + """ + lock_zone = None + if zone: + zone.refresh_from_db() + if zone.status not in STATUS_FINISHED: + lock_zone = zone # Exclude zone if already finished (see #1909) + cls._raise_flow_exception( + LOCK_FAIL_MSG + ': ' + str(ex_msg), + tl_event, + lock_zone, + ) + @classmethod def get_flow( cls, @@ -137,10 +161,8 @@ def run_flow( # Acquire lock coordinator = lock_api.get_coordinator() if not coordinator: - cls._raise_flow_exception( - LOCK_FAIL_MSG + ': Failed to retrieve lock coordinator', - tl_event, - zone, + cls._raise_lock_exception( + 'Failed to retrieve lock coordinator', tl_event, zone ) else: lock_id = str(project.sodar_uuid) @@ -148,11 +170,7 @@ def run_flow( try: lock_api.acquire(lock) except Exception as ex: - cls._raise_flow_exception( - LOCK_FAIL_MSG + ': {}'.format(ex), - tl_event, - zone, - ) + cls._raise_lock_exception(str(ex), tl_event, zone) else: logger.info('Lock not required (flow.require_lock=False)') diff --git a/taskflowbackend/tests/test_flows.py b/taskflowbackend/tests/test_flows.py index 5d8454f4..130f3a84 100644 --- a/taskflowbackend/tests/test_flows.py +++ b/taskflowbackend/tests/test_flows.py @@ -81,6 +81,7 @@ UPDATED_DESC = 'updated description' SCRIPT_USER_NAME = 'script_user' IRODS_ROOT_PATH = 'sodar/root' +INVALID_REDIS_URL = 'redis://127.0.0.1:6666/0' class TaskflowbackendFlowTestBase(TaskflowViewTestBase): @@ -454,7 +455,7 @@ def setUp(self): self.make_irods_colls(self.investigation) def test_delete(self): - """Test landing_zone_delete with an empty landing zone""" + """Test landing_zone_delete with empty landing zone""" zone = self.make_landing_zone( title=ZONE_TITLE, project=self.project, @@ -561,6 +562,54 @@ def test_delete_files_restrict(self): self.assertEqual(self.irods.data_objects.exists(obj_path), False) self.assertEqual(self.irods.collections.exists(zone_path), False) + def test_delete_finished(self): + """Test landing_zone_delete with finished zone""" + # NOTE: This may happen with concurrent requests. See #1909, #1910 + zone = self.make_landing_zone( + title=ZONE_TITLE, + project=self.project, + user=self.user, + assay=self.assay, + description=ZONE_DESC, + status=ZONE_STATUS_DELETED, + ) + # Do not create in taskflow + self.assertEqual(zone.status, ZONE_STATUS_DELETED) + flow_data = {'zone_uuid': str(zone.sodar_uuid)} + flow = self.taskflow.get_flow( + irods_backend=self.irods_backend, + project=self.project, + flow_name='landing_zone_delete', + flow_data=flow_data, + ) + self._build_and_run(flow) + zone.refresh_from_db() + # This should still be deleted, not failed + self.assertEqual(zone.status, ZONE_STATUS_DELETED) + + @override_settings(REDIS_URL=INVALID_REDIS_URL) + def test_delete_finished_lock_failure(self): + """Test landing_zone_delete with finished zone and lock failure""" + zone = self.make_landing_zone( + title=ZONE_TITLE, + project=self.project, + user=self.user, + assay=self.assay, + description=ZONE_DESC, + status=ZONE_STATUS_DELETED, + ) + self.assertEqual(zone.status, ZONE_STATUS_DELETED) + flow_data = {'zone_uuid': str(zone.sodar_uuid)} + flow = self.taskflow.get_flow( + irods_backend=self.irods_backend, + project=self.project, + flow_name='landing_zone_delete', + flow_data=flow_data, + ) + self._build_and_run(flow) + zone.refresh_from_db() + self.assertEqual(zone.status, ZONE_STATUS_DELETED) + class TestLandingZoneMove( LandingZoneMixin,