From 743bf66013df3e7c7a539beeab027c7bd2ffb1cf Mon Sep 17 00:00:00 2001 From: rosemcc Date: Wed, 3 Jun 2026 09:02:28 +1200 Subject: [PATCH 01/13] s3 metadata - handle multiple project owners --- src/service/projectdb_helpers.py | 11 +++++++---- src/workers/submission_worker.py | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/service/projectdb_helpers.py b/src/service/projectdb_helpers.py index 1482318..dc1f8f9 100644 --- a/src/service/projectdb_helpers.py +++ b/src/service/projectdb_helpers.py @@ -78,9 +78,12 @@ def filter_member_identities(members: list[dict[str, Any]]) -> list[dict[str, An return members -def get_project_owner_email(members: Any) -> str: - """Get the project owner's email from the members list.""" +def get_project_owner_emails(members: Any) -> list[str]: + """Get the project owner emails from the members list.""" + owner_emails = [] for member in members: if member["role"]["name"] == "Project Owner": - return member["person"]["email"] or "Unknown" - return "Unknown" + email = member["person"]["email"] + if email: + owner_emails.append(email) + return owner_emails or ["Unknown"] diff --git a/src/workers/submission_worker.py b/src/workers/submission_worker.py index 3cbd0f9..0687242 100644 --- a/src/workers/submission_worker.py +++ b/src/workers/submission_worker.py @@ -26,7 +26,7 @@ upload_file, ) from service.projectdb_client import ProjectDBClient -from service.projectdb_helpers import filter_member_identities, get_project_owner_email +from service.projectdb_helpers import filter_member_identities, get_project_owner_emails from utils.logging import elapsed_ms, log_event from utils.paths import resolve_archive_output_location, resolve_drive_path_for_archive from workers import parse_part_keys_json @@ -476,7 +476,7 @@ async def generate_ro_crate( # pylint: disable=too-many-locals,too-many-stateme timeout=settings.activescale_upload_timeout, metadata={ "cer_project_id": str(project_data.get("id", "")), - "project_owner": get_project_owner_email(members_list), + "project_owners": json.dumps(get_project_owner_emails(members_list)), "division": project_data.get("division") or "Unknown", "data_classification": submission.data_classification or "Unknown", From 021267d4e167e821c64b9e8a8740113a03d9843a Mon Sep 17 00:00:00 2001 From: rosemcc Date: Wed, 3 Jun 2026 11:29:07 +1200 Subject: [PATCH 02/13] mvp - dont use async --- src/workers/retrieval_worker.py | 12 +++----- src/workers/submission_worker.py | 10 ++----- tests/test_chunked_workflow_integration.py | 32 +++++++++------------- 3 files changed, 19 insertions(+), 35 deletions(-) diff --git a/src/workers/retrieval_worker.py b/src/workers/retrieval_worker.py index f14f489..b1ce587 100644 --- a/src/workers/retrieval_worker.py +++ b/src/workers/retrieval_worker.py @@ -2,11 +2,11 @@ from __future__ import annotations -import asyncio import json import logging import shutil import tarfile +import time from datetime import datetime, timedelta from pathlib import Path @@ -70,7 +70,7 @@ def _persist_retrieved_part_keys( session.commit() -async def run_archive_retrieval( # pylint: disable=too-many-statements,too-many-locals,too-many-branches +def run_archive_retrieval( # pylint: disable=too-many-statements,too-many-locals,too-many-branches retrieval_id: int, ) -> None: """Background task: restore, download, and extract a completed archive. @@ -85,10 +85,6 @@ async def run_archive_retrieval( # pylint: disable=too-many-statements,too-many clean up temp files. 4. COMPLETED / FAILED - Final state written to the ArchiveRetrieval record. """ - # Yield to the event loop so uvicorn can flush the HTTP response to the client - # before this blocking-heavy task begins. - await asyncio.sleep(0) - started_at = datetime.now() settings = get_settings() @@ -183,7 +179,7 @@ async def run_archive_retrieval( # pylint: disable=too-many-statements,too-many poll_interval_seconds=poll_interval, elapsed_ms=elapsed_ms(started_at), ) - await asyncio.sleep(poll_interval) + time.sleep(poll_interval) with get_activescale_client_context() as client: if not download_file_to_disk( @@ -252,7 +248,7 @@ async def run_archive_retrieval( # pylint: disable=too-many-statements,too-many poll_interval_seconds=poll_interval, elapsed_ms=elapsed_ms(started_at), ) - await asyncio.sleep(poll_interval) + time.sleep(poll_interval) # ─── Phase 2: DOWNLOADING ───────────────────────────────────────── _transition_retrieval_stage( diff --git a/src/workers/submission_worker.py b/src/workers/submission_worker.py index 0687242..be6ddc7 100644 --- a/src/workers/submission_worker.py +++ b/src/workers/submission_worker.py @@ -2,7 +2,6 @@ from __future__ import annotations -import asyncio import json import logging import shutil @@ -228,12 +227,12 @@ def build_crate_contents( # pylint: disable=too-many-arguments, too-many-positi ) -async def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,too-many-branches +def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,too-many-branches drive: dict[str, Any], submission_id: int, projectdb_client: ProjectDBClient, ) -> None: - """Async background task for generating RO-Crate and updating archive record. + """Background task for generating RO-Crate and updating archive record. Fetches live project data from ProjectDB, generates crate, uploads the archive to ActiveScale for long-term storage, and updates @@ -249,11 +248,6 @@ async def generate_ro_crate( # pylint: disable=too-many-locals,too-many-stateme submission_id: ID of the ArchiveSubmission record projectdb_client: Client for interacting with ProjectDB """ - # Yield to the event loop so uvicorn can flush the HTTP response to the client - # before this blocking-heavy task runs. Without this, the SelectorEventLoop on - # Linux holds the response in its write buffer until we return. - await asyncio.sleep(0) - drive_name = drive.get("name", None) started_at = datetime.now() if drive_name is None: diff --git a/tests/test_chunked_workflow_integration.py b/tests/test_chunked_workflow_integration.py index 672c7c3..c1a5382 100644 --- a/tests/test_chunked_workflow_integration.py +++ b/tests/test_chunked_workflow_integration.py @@ -2,7 +2,6 @@ from __future__ import annotations -import asyncio import json from collections.abc import Generator from contextlib import contextmanager @@ -139,12 +138,11 @@ def fake_upload( monkeypatch.setattr("workers.submission_worker.upload_file", fake_upload) monkeypatch.setattr("workers.submission_worker.object_exists", lambda *_args, **_kwargs: (False, None)) - asyncio.run( - generate_ro_crate( - drive={"id": 1, "name": drive_name}, - submission_id=submission_id, - projectdb_client=_ProjectDbStub(), - ) + + generate_ro_crate( + drive={"id": 1, "name": drive_name}, + submission_id=submission_id, + projectdb_client=_ProjectDbStub(), ) with Session(test_engine) as session: @@ -230,12 +228,10 @@ def fail_on_second_part( monkeypatch.setattr("workers.submission_worker.upload_file", fail_on_second_part) monkeypatch.setattr("workers.submission_worker.object_exists", lambda *_args, **_kwargs: (False, None)) - asyncio.run( - generate_ro_crate( - drive={"id": 1, "name": drive_name}, - submission_id=submission_id, - projectdb_client=_ProjectDbStub(), - ) + generate_ro_crate( + drive={"id": 1, "name": drive_name}, + submission_id=submission_id, + projectdb_client=_ProjectDbStub(), ) with Session(test_engine) as session: @@ -274,12 +270,10 @@ def exists_if_previously_uploaded(_client, _bucket: str, key: str): monkeypatch.setattr("workers.submission_worker.object_exists", exists_if_previously_uploaded) - asyncio.run( - generate_ro_crate( - drive={"id": 1, "name": drive_name}, - submission_id=submission_id, - projectdb_client=_ProjectDbStub(), - ) + generate_ro_crate( + drive={"id": 1, "name": drive_name}, + submission_id=submission_id, + projectdb_client=_ProjectDbStub(), ) with Session(test_engine) as session: From b17a0854a5e6009e8612f62c38cad26b346aaeeb Mon Sep 17 00:00:00 2001 From: rosemcc Date: Thu, 11 Jun 2026 13:33:13 +1200 Subject: [PATCH 03/13] enable boto3 full logs if in debug mode --- src/service/activescale.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/service/activescale.py b/src/service/activescale.py index dbcbfe3..a8f8af0 100644 --- a/src/service/activescale.py +++ b/src/service/activescale.py @@ -112,6 +112,9 @@ def _create_activescale_session() -> boto3.Session: "ActiveScale credentials are not fully set in environment variables." ) + if settings.log_level == logging.DEBUG: + boto3.set_stream_logger('') + session = boto3.Session( aws_access_key_id=access_key, aws_secret_access_key=secret_key, From d606a20e0b48a1d234ae4c4341b40e3724267092 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Fri, 12 Jun 2026 13:20:46 +1200 Subject: [PATCH 04/13] post-upload verify ContentLength matches expected size --- src/service/activescale.py | 63 ++++++++++++++++++++ src/workers/submission_worker.py | 18 +++++- tests/test_activescale.py | 68 ++++++++++++++++++++++ tests/test_chunked_upload.py | 98 ++++++++++++++++++++++++++++++++ 4 files changed, 246 insertions(+), 1 deletion(-) create mode 100644 tests/test_activescale.py diff --git a/src/service/activescale.py b/src/service/activescale.py index a8f8af0..a7fa0d0 100644 --- a/src/service/activescale.py +++ b/src/service/activescale.py @@ -520,6 +520,69 @@ def object_exists( return False, None +def verify_uploaded_part_size( + client: S3Client, + bucket_name: str, + file_key: str, + expected_size: int, +) -> bool: + """Verify that an uploaded object's size matches the local file size. + + Issues a ``head_object`` call and compares ``ContentLength`` against + *expected_size*. + + Args: + client: An initialized S3 client. + bucket_name: Name of the S3 bucket. + file_key: Object key to verify. + expected_size: Expected size in bytes (as recorded in the archive manifest). + + Returns: + True if ``ContentLength`` equals *expected_size*, False otherwise + (including on any error, so callers can treat False as a hard failure). + """ + try: + response = client.head_object(Bucket=bucket_name, Key=file_key) + actual_size = response.get("ContentLength", -1) + if actual_size != expected_size: + _log_event( + logging.ERROR, + "s3.object.size_mismatch", + file_key=file_key, + bucket_name=bucket_name, + expected_size=expected_size, + actual_size=actual_size, + ) + return False + _log_event( + logging.INFO, + "s3.object.size_verified", + file_key=file_key, + bucket_name=bucket_name, + size_bytes=actual_size, + ) + return True + except ClientError as e: + _log_client_error( + "s3.object.size_verify.client_error", + e, + file_key=file_key, + bucket_name=bucket_name, + ) + return False + except EndpointConnectionError: + _log_endpoint_connection_error(file_key=file_key, bucket_name=bucket_name) + return False + except (BotoCoreError, OSError, ValueError, TypeError) as e: + _log_unexpected_error( + "s3.object.size_verify.unexpected_error", + e, + file_key=file_key, + bucket_name=bucket_name, + ) + return False + + def create_bucket( client: S3Client, bucket_name: str, diff --git a/src/workers/submission_worker.py b/src/workers/submission_worker.py index be6ddc7..a3a8dc8 100644 --- a/src/workers/submission_worker.py +++ b/src/workers/submission_worker.py @@ -15,7 +15,7 @@ from config import get_settings from models.common import calculate_retention_end_date from models.submission import ArchiveJobStage, ArchiveSubmission -from packaging.archive_chunks import build_chunked_tar_archive +from packaging.archive_chunks import ArchivePartInfo, build_chunked_tar_archive from packaging.crate.ro_builder import ROBuilder from packaging.crate.ro_loader import ROLoader from packaging.manifests import bag_directory, bagit_exists, create_manifests_directory @@ -23,6 +23,7 @@ get_activescale_client_context, object_exists, upload_file, + verify_uploaded_part_size, ) from service.projectdb_client import ProjectDBClient from service.projectdb_helpers import filter_member_identities, get_project_owner_emails @@ -103,6 +104,7 @@ def _upload_chunked_archive_parts( # pylint: disable=too-many-arguments bucket_name: str, object_prefix: str, archive_parts_dir: Path, + archive_parts: list[ArchivePartInfo], timeout_seconds: int, ) -> tuple[bool, list[str]]: """Upload chunked archive part files with resume support. @@ -113,6 +115,7 @@ def _upload_chunked_archive_parts( # pylint: disable=too-many-arguments """ part_files = sorted(archive_parts_dir.glob("*.tar.gz.part-*")) uploaded_keys = parse_part_keys_json(submission.archive_part_keys_json) + manifest_sizes: dict[str, int] = {p.file_name: p.size_bytes for p in archive_parts} for part_file in part_files: part_key = f"{object_prefix}{part_file.name}" @@ -146,6 +149,18 @@ def _upload_chunked_archive_parts( # pylint: disable=too-many-arguments ) return False, uploaded_keys + expected_size = manifest_sizes.get(part_file.name, part_file.stat().st_size) + if not verify_uploaded_part_size(client, bucket_name, part_key, expected_size): + log_event( + logging.ERROR, + "crate.upload.part.size_mismatch", + submission_id=submission.id, + drive_name=submission.drive_name, + part_key=part_key, + expected_size=expected_size, + ) + return False, uploaded_keys + uploaded_keys.append(part_key) _persist_uploaded_part_keys(session, submission, uploaded_keys) log_event( @@ -438,6 +453,7 @@ def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,to bucket_name=bucket_name, object_prefix=object_prefix, archive_parts_dir=archive_parts_dir, + archive_parts=chunk_result.parts, timeout_seconds=settings.activescale_upload_timeout, ) diff --git a/tests/test_activescale.py b/tests/test_activescale.py new file mode 100644 index 0000000..a4d6537 --- /dev/null +++ b/tests/test_activescale.py @@ -0,0 +1,68 @@ +"""Unit tests for activescale S3 integration helpers.""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest +from botocore.exceptions import BotoCoreError, ClientError, EndpointConnectionError + +from service.activescale import verify_uploaded_part_size + + +def _make_client_error(code: str) -> ClientError: + return ClientError( + {"Error": {"Code": code, "Message": "test error"}}, "HeadObject" + ) + + +class TestVerifyUploadedPartSize: + def test_returns_true_when_sizes_match(self) -> None: + client = MagicMock() + client.head_object.return_value = {"ContentLength": 512} + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is True + client.head_object.assert_called_once_with(Bucket="bucket", Key="key/part-00001") + + def test_returns_false_when_sizes_differ(self) -> None: + client = MagicMock() + client.head_object.return_value = {"ContentLength": 100} + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is False + + def test_returns_false_when_content_length_absent(self) -> None: + # head_object response missing ContentLength — treated as -1 != expected + client = MagicMock() + client.head_object.return_value = {} + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is False + + def test_returns_false_on_client_error(self) -> None: + client = MagicMock() + client.head_object.side_effect = _make_client_error("403") + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is False + + def test_returns_false_on_404(self) -> None: + client = MagicMock() + client.head_object.side_effect = _make_client_error("404") + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is False + + def test_returns_false_on_endpoint_connection_error(self) -> None: + client = MagicMock() + client.head_object.side_effect = EndpointConnectionError(endpoint_url="https://example.com") + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is False + + def test_returns_false_on_botocore_error(self) -> None: + client = MagicMock() + client.head_object.side_effect = BotoCoreError() + + assert verify_uploaded_part_size(client, "bucket", "key/part-00001", 512) is False + + def test_zero_byte_object(self) -> None: + client = MagicMock() + client.head_object.return_value = {"ContentLength": 0} + + assert verify_uploaded_part_size(client, "bucket", "key/empty", 0) is True diff --git a/tests/test_chunked_upload.py b/tests/test_chunked_upload.py index c0d79dc..e41d6bc 100644 --- a/tests/test_chunked_upload.py +++ b/tests/test_chunked_upload.py @@ -10,6 +10,7 @@ from workers import parse_part_keys_json from workers.submission_worker import _upload_chunked_archive_parts +from packaging.archive_chunks import ArchivePartInfo from models.common import DataClassification from models.submission import ArchiveSubmission @@ -72,6 +73,9 @@ def fake_upload(_client, _bucket: str, key: str, file_path: str, timeout: int): monkeypatch.setattr("workers.submission_worker.object_exists", fake_exists) monkeypatch.setattr("workers.submission_worker.upload_file", fake_upload) + monkeypatch.setattr( + "workers.submission_worker.verify_uploaded_part_size", lambda *_a, **_k: True + ) success, result_keys = _upload_chunked_archive_parts( session=session, @@ -80,6 +84,10 @@ def fake_upload(_client, _bucket: str, key: str, file_path: str, timeout: int): bucket_name="bucket", object_prefix=prefix, archive_parts_dir=archive_parts_dir, + archive_parts=[ + ArchivePartInfo(index=1, file_name=first.name, size_bytes=len(b"part1"), sha256="a"), + ArchivePartInfo(index=2, file_name=second.name, size_bytes=len(b"part2"), sha256="b"), + ], timeout_seconds=60, ) @@ -114,9 +122,99 @@ def test_upload_chunked_parts_stops_on_failure( bucket_name="bucket", object_prefix=prefix, archive_parts_dir=archive_parts_dir, + archive_parts=[ + ArchivePartInfo(index=1, file_name=first.name, size_bytes=len(b"part1"), sha256="a"), + ], timeout_seconds=60, ) assert success is False assert result_keys == [] assert expected_key not in result_keys + + +def test_upload_chunked_parts_fails_on_size_mismatch( + tmp_path: Path, + session: Session, + monkeypatch, +) -> None: + """Upload succeeds but post-upload size check fails → job aborts.""" + archive_parts_dir = tmp_path / "parts" + archive_parts_dir.mkdir(parents=True, exist_ok=True) + part = archive_parts_dir / "drive.tar.gz.part-00001" + part.write_bytes(b"part1") + + prefix = "drive/" + part_key = f"{prefix}{part.name}" + + submission = _create_submission(session, drive_name="resmed202200024-testing") + + monkeypatch.setattr("workers.submission_worker.object_exists", lambda *_a, **_k: (False, None)) + monkeypatch.setattr("workers.submission_worker.upload_file", lambda *_a, **_k: True) + monkeypatch.setattr( + "workers.submission_worker.verify_uploaded_part_size", lambda *_a, **_k: False + ) + + success, result_keys = _upload_chunked_archive_parts( + session=session, + submission=submission, + client=object(), + bucket_name="bucket", + object_prefix=prefix, + archive_parts_dir=archive_parts_dir, + archive_parts=[ + ArchivePartInfo(index=1, file_name=part.name, size_bytes=len(b"part1"), sha256="a"), + ], + timeout_seconds=60, + ) + + assert success is False + # Part must not be recorded as successfully uploaded when size check fails + assert part_key not in result_keys + + +def test_upload_chunked_parts_size_check_called_with_correct_args( + tmp_path: Path, + session: Session, + monkeypatch, +) -> None: + """verify_uploaded_part_size is called with the correct key and file size.""" + archive_parts_dir = tmp_path / "parts" + archive_parts_dir.mkdir(parents=True, exist_ok=True) + part = archive_parts_dir / "drive.tar.gz.part-00001" + part_content = b"hello archive" + part.write_bytes(part_content) + + prefix = "drive/" + part_key = f"{prefix}{part.name}" + + submission = _create_submission(session, drive_name="resmed202200024-testing") + + size_check_calls: list[tuple] = [] + + def capture_size_check(_client, _bucket: str, key: str, expected_size: int) -> bool: + size_check_calls.append((key, expected_size)) + return True + + manifest_size = 999 # deliberately different from len(part_content) to prove manifest wins + + monkeypatch.setattr("workers.submission_worker.object_exists", lambda *_a, **_k: (False, None)) + monkeypatch.setattr("workers.submission_worker.upload_file", lambda *_a, **_k: True) + monkeypatch.setattr("workers.submission_worker.verify_uploaded_part_size", capture_size_check) + + success, _ = _upload_chunked_archive_parts( + session=session, + submission=submission, + client=object(), + bucket_name="bucket", + object_prefix=prefix, + archive_parts_dir=archive_parts_dir, + archive_parts=[ + ArchivePartInfo(index=1, file_name=part.name, size_bytes=manifest_size, sha256="a"), + ], + timeout_seconds=60, + ) + + assert success is True + assert len(size_check_calls) == 1 + assert size_check_calls[0] == (part_key, manifest_size) From a7f8597130b19a48d2645ce506a51409afd893b7 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Fri, 12 Jun 2026 13:46:00 +1200 Subject: [PATCH 05/13] pre-upload tar validation - detect truncated parts, corrupted gzip or tar structural issues --- src/packaging/archive_chunks.py | 90 ++++++++++++++++++++++ src/workers/submission_worker.py | 22 +++++- tests/test_archive_chunks.py | 79 ++++++++++++++++++- tests/test_chunked_workflow_integration.py | 4 +- 4 files changed, 192 insertions(+), 3 deletions(-) diff --git a/src/packaging/archive_chunks.py b/src/packaging/archive_chunks.py index fdcec4b..94ac23f 100644 --- a/src/packaging/archive_chunks.py +++ b/src/packaging/archive_chunks.py @@ -166,6 +166,96 @@ def _finalize_current_part(self) -> None: self._current_size = 0 +class _ChainReader: + """Read sequentially across an ordered list of part files without loading them into memory. + + Presents a file-like ``read()`` interface so the concatenated byte stream + can be passed directly to :func:`tarfile.open` without first assembling a + single file on disk. + """ + + def __init__(self, parts: list[ArchivePartInfo], parts_dir: Path) -> None: + self._paths = [ + parts_dir / p.file_name for p in sorted(parts, key=lambda p: p.index) + ] + self._file_index = 0 + self._current_fp: BinaryIO | None = None + + def read(self, size: int = -1) -> bytes: + """Read up to *size* bytes across part boundaries, or all remaining bytes if -1.""" + if size == 0: + return b"" + + buf = bytearray() + remaining = size # -1 means read everything + + while True: + if self._current_fp is None: + if self._file_index >= len(self._paths): + break + self._current_fp = ( + open( # noqa: SIM115 # pylint: disable=consider-using-with + self._paths[self._file_index], "rb" + ) + ) + self._file_index += 1 + + chunk = self._current_fp.read(remaining if remaining != -1 else -1) + if chunk: + buf.extend(chunk) + if remaining != -1: + remaining -= len(chunk) + if remaining == 0: + break + else: + # Current file exhausted — move to next + self._current_fp.close() + self._current_fp = None + + return bytes(buf) + + def close(self) -> None: + """Close any open file handle.""" + if self._current_fp is not None: + self._current_fp.close() + self._current_fp = None + + def __enter__(self) -> _ChainReader: + return self + + def __exit__(self, *_: object) -> None: + self.close() + + +def verify_tar_parts_stream(parts: list[ArchivePartInfo], parts_dir: Path) -> None: + """Verify the integrity of a chunked tar.gz archive by streaming all parts. + + Chains the ordered part files into a single logical byte stream and passes + it to :func:`tarfile.open` in streaming read mode (``r|gz``). Iterating + :meth:`~tarfile.TarFile.getmembers` forces full decompression and gzip CRC + validation without writing anything to disk. + + Raises: + FileNotFoundError: If any part file is missing. + tarfile.TarError: If the gzip stream is corrupt or the tar structure is invalid. + """ + for part in parts: + part_path = parts_dir / part.file_name + if not part_path.exists(): + raise FileNotFoundError(f"Archive part file not found: {part_path}") + + with _ChainReader(parts, parts_dir) as chain: + with tarfile.open(fileobj=cast(BinaryIO, chain), mode="r|gz") as tar: + member_count = 0 + for _ in tar: + member_count += 1 + + if member_count == 0: + raise tarfile.TarError( + "Tar stream contained no members — archive may be empty or corrupt" + ) + + def build_chunked_tar_archive( source_dir: Path, output_dir: Path, diff --git a/src/workers/submission_worker.py b/src/workers/submission_worker.py index a3a8dc8..448bf25 100644 --- a/src/workers/submission_worker.py +++ b/src/workers/submission_worker.py @@ -15,7 +15,7 @@ from config import get_settings from models.common import calculate_retention_end_date from models.submission import ArchiveJobStage, ArchiveSubmission -from packaging.archive_chunks import ArchivePartInfo, build_chunked_tar_archive +from packaging.archive_chunks import ArchivePartInfo, build_chunked_tar_archive, verify_tar_parts_stream from packaging.crate.ro_builder import ROBuilder from packaging.crate.ro_loader import ROLoader from packaging.manifests import bag_directory, bagit_exists, create_manifests_directory @@ -413,6 +413,26 @@ def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,to elapsed_ms=elapsed_ms(started_at), ) + log_event( + logging.INFO, + "crate.package.tar_verify.start", + submission_id=submission_id, + drive_name=drive_name, + part_count=len(chunk_result.parts), + elapsed_ms=elapsed_ms(started_at), + ) + verify_tar_parts_stream( + parts=chunk_result.parts, + parts_dir=archive_parts_dir, + ) + log_event( + logging.INFO, + "crate.package.tar_verify.completed", + submission_id=submission_id, + drive_name=drive_name, + elapsed_ms=elapsed_ms(started_at), + ) + # Transition: packaging → uploading previous_stage = submission.stage submission.stage = ArchiveJobStage.UPLOADING diff --git a/tests/test_archive_chunks.py b/tests/test_archive_chunks.py index c72e013..f929fda 100644 --- a/tests/test_archive_chunks.py +++ b/tests/test_archive_chunks.py @@ -6,8 +6,9 @@ import tarfile from pathlib import Path -from packaging.archive_chunks import build_chunked_tar_archive +import pytest +from packaging.archive_chunks import build_chunked_tar_archive, verify_tar_parts_stream def _write_file(path: Path, size: int) -> None: path.parent.mkdir(parents=True, exist_ok=True) @@ -68,3 +69,79 @@ def test_chunked_parts_reassemble_into_valid_tar(tmp_path: Path) -> None: assert any(name.endswith("source/one.txt") for name in names) assert any(name.endswith("source/two.txt") for name in names) + + +# ── verify_tar_parts_stream ────────────────────────────────────────────────── + + +def test_verify_tar_parts_stream_passes_for_valid_archive(tmp_path: Path) -> None: + source_dir = tmp_path / "source" + _write_file(source_dir / "a.txt", 1000) + _write_file(source_dir / "b.txt", 1000) + + output_dir = tmp_path / "output" + result = build_chunked_tar_archive( + source_dir=source_dir, + output_dir=output_dir, + base_name="drive-archive", + part_size_bytes=500, + ) + + # Should not raise + verify_tar_parts_stream(parts=result.parts, parts_dir=output_dir) + + +def test_verify_tar_parts_stream_raises_on_missing_part(tmp_path: Path) -> None: + source_dir = tmp_path / "source" + _write_file(source_dir / "a.txt", 1000) + + output_dir = tmp_path / "output" + result = build_chunked_tar_archive( + source_dir=source_dir, + output_dir=output_dir, + base_name="drive-archive", + part_size_bytes=300, + ) + + # Delete the first part + (output_dir / result.parts[0].file_name).unlink() + + with pytest.raises(FileNotFoundError, match="Archive part file not found"): + verify_tar_parts_stream(parts=result.parts, parts_dir=output_dir) + + +def test_verify_tar_parts_stream_raises_on_corrupt_part(tmp_path: Path) -> None: + source_dir = tmp_path / "source" + _write_file(source_dir / "a.txt", 2000) + + output_dir = tmp_path / "output" + result = build_chunked_tar_archive( + source_dir=source_dir, + output_dir=output_dir, + base_name="drive-archive", + part_size_bytes=400, + ) + + # Overwrite the last part with garbage to corrupt the gzip stream + last_part = output_dir / result.parts[-1].file_name + last_part.write_bytes(b"\xff" * last_part.stat().st_size) + + with pytest.raises(tarfile.TarError): + verify_tar_parts_stream(parts=result.parts, parts_dir=output_dir) + + +def test_verify_tar_parts_stream_single_part(tmp_path: Path) -> None: + """Works correctly when the archive fits in a single part.""" + source_dir = tmp_path / "source" + _write_file(source_dir / "small.txt", 50) + + output_dir = tmp_path / "output" + result = build_chunked_tar_archive( + source_dir=source_dir, + output_dir=output_dir, + base_name="drive-archive", + part_size_bytes=512 * 1024 * 1024, # 512 MB — file will be one part + ) + + assert len(result.parts) == 1 + verify_tar_parts_stream(parts=result.parts, parts_dir=output_dir) diff --git a/tests/test_chunked_workflow_integration.py b/tests/test_chunked_workflow_integration.py index c1a5382..cc12bf6 100644 --- a/tests/test_chunked_workflow_integration.py +++ b/tests/test_chunked_workflow_integration.py @@ -137,8 +137,8 @@ def fake_upload( monkeypatch.setattr("workers.submission_worker.upload_file", fake_upload) monkeypatch.setattr("workers.submission_worker.object_exists", lambda *_args, **_kwargs: (False, None)) + monkeypatch.setattr("workers.submission_worker.verify_uploaded_part_size", lambda *_args, **_kwargs: True) - generate_ro_crate( drive={"id": 1, "name": drive_name}, submission_id=submission_id, @@ -227,6 +227,7 @@ def fail_on_second_part( monkeypatch.setattr("workers.submission_worker.upload_file", fail_on_second_part) monkeypatch.setattr("workers.submission_worker.object_exists", lambda *_args, **_kwargs: (False, None)) + monkeypatch.setattr("workers.submission_worker.verify_uploaded_part_size", lambda *_args, **_kwargs: True) generate_ro_crate( drive={"id": 1, "name": drive_name}, @@ -264,6 +265,7 @@ def upload_all( return True monkeypatch.setattr("workers.submission_worker.upload_file", upload_all) + monkeypatch.setattr("workers.submission_worker.verify_uploaded_part_size", lambda *_args, **_kwargs: True) def exists_if_previously_uploaded(_client, _bucket: str, key: str): return (key in first_run_uploaded), None From 173a9883ddf5369ecb248affdff6feaa6dbdb7be Mon Sep 17 00:00:00 2001 From: rosemcc Date: Wed, 17 Jun 2026 13:23:06 +1200 Subject: [PATCH 06/13] gitignore, todos --- .gitignore | 4 +++- TODO.md | 14 +++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index a76ee50..f57abd1 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,6 @@ api_keys.json venv -vite.config.ts.timestamp* \ No newline at end of file +vite.config.ts.timestamp* + +data_examples/* \ No newline at end of file diff --git a/TODO.md b/TODO.md index fd58b50..be23e7b 100644 --- a/TODO.md +++ b/TODO.md @@ -2,12 +2,12 @@ ## Scale and Performance - [ ] Add configurable limits for archive jobs (for example max files, max bytes, max runtime) and enforce them in backend checks. -- [ ] Optimise for large drives and high file counts (streaming, batching, incremental processing or splitting into multiple RO-Crates). Max file size for ActiveScale is 50TB. +- [x] Optimise for large drives and high file counts (streaming, batching, incremental processing or splitting into multiple RO-Crates). Max file size for ActiveScale is 50TB. - [ ] Reassess background execution approach; replace FastAPI `BackgroundTasks` with a durable queue (such as Celery or RQ) if reliability requirements increase. ## Large Archive Datamodel and API (Draft) - [x] Extend `ArchiveSubmission` schema to track archive transport metadata (`archive_part_count`, byte sizes, object prefix, manifest key, and ordered part keys). -- [x] Extend `JobStage` vocabulary for chunked archive workflow (`packaging`, `uploading`, `writing_manifest`) while retaining legacy stage values for compatibility. +- [x] Extend `ArchiveJobStage` vocabulary for chunked archive workflow (`packaging`, `uploading`, `writing_manifest`) while retaining legacy stage values for compatibility. - [x] Extend `GET /api/v1/submission` response payload with new archive transport fields. - [x] Implement chunked archive writer (single logical tar split into ordered parts below ActiveScale object limit). - [x] Upload each part as a separate object under a deterministic prefix and persist uploaded part keys incrementally. @@ -20,4 +20,12 @@ ## Quality and Validation - [ ] Improve end-to-end tests that cover submission -> manifest -> RO-Crate build -> upload flow. -- [ ] Validate generated RO-Crate output against the profile as part of CI. +- [ ] Validate generated RO-Crate output against the profile. +- [ ] Improved object and Tar integrity checks (e.g. validate checksums of uploaded parts, verify manifest integrity, and ensure reassembled archive matches original input). +- [ ] Question: should custom s3 metadata be added on every uploaded object/prat of the archive? Currently just the archive manifest object has the metadata added. + +## Features and Enhancements +- [x] Add archive retrieval endpoint that reassembles chunked archive on-the-fly and uploads it into a Vast view. This would allow admins to retrieve a researchers' archived data without needing to interact directly with ActiveScale or S3 APIs. It needs to put the reassembled archive into a Vast view (i.e. back into the drive namespace that it came from) for users to access with existing tools. The workflow would be: researcher asks for archive retrieval -> administrator creates vast view with the research drive name -> administrator sends request to backend -> backend retrieves manifest (NOTE: object retrieval might require command restore-object to 'thaw' the data to disk before download. This can take time and so will need to poll s3 for the object status to change from archived to restored.) -> backend streams chunked archive parts from ActiveScale into Vast -> backend validates checksums andreassembles the archive -> backend extracts files and validates bagIt -> notify administrator that job is completed (and whether success/failure) -> researcher accesses the view with their tools. This would require careful handling of streaming and temporary storage to avoid memory issues with large archives. Question: should a record be stored in projectdb for retrieval jobs to track their status and metadata? +- [ ] Notifications module to send slack messages to admins when jobs complete or fail +- [ ] Logging to a durable store (e.g. file, database, or logging service) instead of just stdout for better traceability and debugging. +- [x] Refactor main.py to separate API route definitions from core business logic / background tasks. From 9f7a2fb610799afa0232b3b9d43d33d0873554c3 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Wed, 17 Jun 2026 16:10:15 +1200 Subject: [PATCH 07/13] update fastapi --- poetry.lock | 8 ++++---- pyproject.toml | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/poetry.lock b/poetry.lock index 9e74877..3e41426 100644 --- a/poetry.lock +++ b/poetry.lock @@ -505,14 +505,14 @@ idna = ">=2.0.0" [[package]] name = "fastapi" -version = "0.136.3" +version = "0.137.1" description = "FastAPI framework, high performance, easy to learn, fast to code, ready for production" optional = false python-versions = ">=3.10" groups = ["main"] files = [ - {file = "fastapi-0.136.3-py3-none-any.whl", hash = "sha256:3d2a69bdf04b7e9f3afa292c3bc7a98816bbfafa10bc9b45f3f3700d2f761620"}, - {file = "fastapi-0.136.3.tar.gz", hash = "sha256:e487fae93ad408e6f47641ee4dfe389864fd7bec92e547ea8498fc13f43e83ab"}, + {file = "fastapi-0.137.1-py3-none-any.whl", hash = "sha256:64f6983c59e45c4b9fdc44e57cb8035c2451ee91ea8e8ec042aca37de7cf6b69"}, + {file = "fastapi-0.137.1.tar.gz", hash = "sha256:822360704230d9533d8d9475399613525968aa2f0b5bd2a3ccc9f18c88fd541c"}, ] [package.dependencies] @@ -2827,4 +2827,4 @@ files = [ [metadata] lock-version = "2.1" python-versions = "^3.14" -content-hash = "3dca1b0c1920f8de36c34d10b156af03197b1fa5f2b18f71d69c3a1047d97063" +content-hash = "f4cfbf8ac9b8d82819ada1f08e8745d6c85fbc77e67f09479e406884c1d3e0f6" diff --git a/pyproject.toml b/pyproject.toml index 2678a73..e2a8774 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,7 @@ package-mode = false [tool.poetry.dependencies] python = "^3.14" -fastapi = {extras = ["standard"], version = "^0.136.3"} +fastapi = {extras = ["standard"], version = "^0.137.1"} sqlmodel = "^0.0.22" sqlalchemy = "^2.0.36" bagit = "^1.8.1" From 3ab336039f839ae5df0d6e170fd93b521bc126ac Mon Sep 17 00:00:00 2001 From: rosemcc Date: Fri, 19 Jun 2026 11:45:26 +1200 Subject: [PATCH 08/13] todo update --- TODO.md | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/TODO.md b/TODO.md index be23e7b..c15fed7 100644 --- a/TODO.md +++ b/TODO.md @@ -1,22 +1,9 @@ # TODO ## Scale and Performance -- [ ] Add configurable limits for archive jobs (for example max files, max bytes, max runtime) and enforce them in backend checks. -- [x] Optimise for large drives and high file counts (streaming, batching, incremental processing or splitting into multiple RO-Crates). Max file size for ActiveScale is 50TB. -- [ ] Reassess background execution approach; replace FastAPI `BackgroundTasks` with a durable queue (such as Celery or RQ) if reliability requirements increase. +- [ ] Add configurable limits for archive jobs (for example max files, max bytes, max runtime) and enforce them in backend checks. Especially once we know the limit of whatever the prod infrastructure will be. +- [ ] Reassess background execution approach; replace FastAPI `BackgroundTasks` with a durable queue (such as Celery or RQ) if reliability requirements increase. OR make it so only one archiving job can be run at a time to avoid concurrency issues with the current implementation. -## Large Archive Datamodel and API (Draft) -- [x] Extend `ArchiveSubmission` schema to track archive transport metadata (`archive_part_count`, byte sizes, object prefix, manifest key, and ordered part keys). -- [x] Extend `ArchiveJobStage` vocabulary for chunked archive workflow (`packaging`, `uploading`, `writing_manifest`) while retaining legacy stage values for compatibility. -- [x] Extend `GET /api/v1/submission` response payload with new archive transport fields. -- [x] Implement chunked archive writer (single logical tar split into ordered parts below ActiveScale object limit). -- [x] Upload each part as a separate object under a deterministic prefix and persist uploaded part keys incrementally. -- [x] Write sidecar archive manifest file (`archive-manifest.json`) during packaging with part ordering, per-part checksum, and total byte count. -- [x] Upload sidecar archive manifest object to ActiveScale alongside uploaded parts. -- [x] Replace single-object upload call in archive worker with chunked upload pipeline. -- [x] Add retry/resume support to skip already uploaded parts and continue from persisted metadata. -- [x] Add archive retrieval/reassembly utility using persisted part ordering from manifest. -- [x] Add integration tests for chunked upload success, interrupted upload resume, and manifest integrity checks. ## Quality and Validation - [ ] Improve end-to-end tests that cover submission -> manifest -> RO-Crate build -> upload flow. @@ -25,7 +12,6 @@ - [ ] Question: should custom s3 metadata be added on every uploaded object/prat of the archive? Currently just the archive manifest object has the metadata added. ## Features and Enhancements -- [x] Add archive retrieval endpoint that reassembles chunked archive on-the-fly and uploads it into a Vast view. This would allow admins to retrieve a researchers' archived data without needing to interact directly with ActiveScale or S3 APIs. It needs to put the reassembled archive into a Vast view (i.e. back into the drive namespace that it came from) for users to access with existing tools. The workflow would be: researcher asks for archive retrieval -> administrator creates vast view with the research drive name -> administrator sends request to backend -> backend retrieves manifest (NOTE: object retrieval might require command restore-object to 'thaw' the data to disk before download. This can take time and so will need to poll s3 for the object status to change from archived to restored.) -> backend streams chunked archive parts from ActiveScale into Vast -> backend validates checksums andreassembles the archive -> backend extracts files and validates bagIt -> notify administrator that job is completed (and whether success/failure) -> researcher accesses the view with their tools. This would require careful handling of streaming and temporary storage to avoid memory issues with large archives. Question: should a record be stored in projectdb for retrieval jobs to track their status and metadata? - [ ] Notifications module to send slack messages to admins when jobs complete or fail - [ ] Logging to a durable store (e.g. file, database, or logging service) instead of just stdout for better traceability and debugging. -- [x] Refactor main.py to separate API route definitions from core business logic / background tasks. +- [ ] Add workflow for deleting the original drive data after successful archive. Key steps would be: flagging the source data as ready for deletion, running a separate cleanup job that verifies the archive integrity, and verifies the object exists before deleting, and handling any edge cases (e.g. what if the archive is corrupted?). It would also need to retain a copy of the archive manifest, and location of the stored archive, in the research drive (the drives/views/shares themselves will not be deleted). This may be a separate workflow from the archiving process, but could be triggered from the same API endpoint by adding an additional parameter to indicate whether deletion should be performed after archiving. From e7da7fcc07f67d047c5f23bb3dc6ef44165bd80c Mon Sep 17 00:00:00 2001 From: rosemcc Date: Fri, 19 Jun 2026 11:54:28 +1200 Subject: [PATCH 09/13] lint --- src/service/activescale.py | 4 ++-- src/workers/submission_worker.py | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/service/activescale.py b/src/service/activescale.py index a7fa0d0..6b3670b 100644 --- a/src/service/activescale.py +++ b/src/service/activescale.py @@ -112,8 +112,8 @@ def _create_activescale_session() -> boto3.Session: "ActiveScale credentials are not fully set in environment variables." ) - if settings.log_level == logging.DEBUG: - boto3.set_stream_logger('') + if settings.log_level.upper() == "DEBUG": + boto3.set_stream_logger("") session = boto3.Session( aws_access_key_id=access_key, diff --git a/src/workers/submission_worker.py b/src/workers/submission_worker.py index 448bf25..533bc77 100644 --- a/src/workers/submission_worker.py +++ b/src/workers/submission_worker.py @@ -15,7 +15,11 @@ from config import get_settings from models.common import calculate_retention_end_date from models.submission import ArchiveJobStage, ArchiveSubmission -from packaging.archive_chunks import ArchivePartInfo, build_chunked_tar_archive, verify_tar_parts_stream +from packaging.archive_chunks import ( + ArchivePartInfo, + build_chunked_tar_archive, + verify_tar_parts_stream, +) from packaging.crate.ro_builder import ROBuilder from packaging.crate.ro_loader import ROLoader from packaging.manifests import bag_directory, bagit_exists, create_manifests_directory @@ -506,7 +510,9 @@ def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,to timeout=settings.activescale_upload_timeout, metadata={ "cer_project_id": str(project_data.get("id", "")), - "project_owners": json.dumps(get_project_owner_emails(members_list)), + "project_owners": json.dumps( + get_project_owner_emails(members_list) + ), "division": project_data.get("division") or "Unknown", "data_classification": submission.data_classification or "Unknown", From 9f4735867c8f223bc441f44e081476fc58520088 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Wed, 24 Jun 2026 15:58:24 +1200 Subject: [PATCH 10/13] set object retention period on archive objects --- modes/.env.development | 3 +- src/config.py | 8 ++ src/models/common.py | 20 +++- src/service/activescale.py | 63 +++++++++++ src/workers/submission_worker.py | 70 ++++++++++++- tests/test_activescale.py | 80 +++++++++++++- tests/test_chunked_upload.py | 116 +++++++++++++++++++++ tests/test_chunked_workflow_integration.py | 6 ++ 8 files changed, 358 insertions(+), 8 deletions(-) diff --git a/modes/.env.development b/modes/.env.development index 624fc98..0b2ae2b 100644 --- a/modes/.env.development +++ b/modes/.env.development @@ -7,4 +7,5 @@ ACTIVESCALE_HOSTNAME=asi.s3.prod.nz ACTIVESCALE_REGION=us-east-1 LOG_LEVEL=INFO PROJECTDB_BASE_URL=https://apis.test.auckland.ac.nz/proxy-eresearch-project -ARCHIVE_CHUNK_SIZE_BYTES=54975581388798 # 50 TB \ No newline at end of file +ARCHIVE_CHUNK_SIZE_BYTES=54975581388798 # 50 TB +ACTIVESCALE_RETENTION_OVERRIDE_DAYS=1 \ No newline at end of file diff --git a/src/config.py b/src/config.py index 45fe911..d5272b2 100644 --- a/src/config.py +++ b/src/config.py @@ -63,6 +63,14 @@ class Settings(BaseSettings): activescale_restore_poll_interval_seconds: int = 60 # Maximum total time to wait for a restore to complete, in seconds (default 24 h) activescale_restore_poll_max_seconds: int = 86400 + # Object retention (object lock COMPLIANCE mode) - (default True). + # Set to False in TEST environments so objects can be deleted quickly. + activescale_enable_object_retention: bool = True + # Fallback retention in years when retention_period_years is not set on a submission. + activescale_default_retention_years: int = 10 + # Override: when set, use this many days as the retention period instead of the + # years-based calculation. Intended for TEST environments so objects expire quickly. + activescale_retention_override_days: int | None = None model_config = SettingsConfigDict(env_file=get_env_file(), extra="ignore") diff --git a/src/models/common.py b/src/models/common.py index af713dd..bbe6003 100644 --- a/src/models/common.py +++ b/src/models/common.py @@ -1,7 +1,7 @@ """Classes common to other models.""" import re -from datetime import datetime +from datetime import datetime, timezone from enum import Enum from typing import Annotated @@ -22,6 +22,20 @@ def validate_resdrive_name(drive_name: str) -> str: return drive_name +def calculate_retention_end_datetime( + start_date: datetime, retention_years: int +) -> datetime: + """Return the UTC-aware datetime on which retained data may be deleted. + + Args: + start_date: The starting datetime (will be treated as UTC if naive). + retention_years: Number of full years to add. + """ + if start_date.tzinfo is None: + start_date = start_date.replace(tzinfo=timezone.utc) + return start_date + relativedelta(years=retention_years) + + def calculate_retention_end_date(start_date: datetime, retention_years: int) -> str: """Return the date on which retained data may be deleted. @@ -29,7 +43,9 @@ def calculate_retention_end_date(start_date: datetime, retention_years: int) -> start_date: The starting date (typically project end date or today). retention_years: Number of full years to add. """ - return (start_date + relativedelta(years=retention_years)).strftime("%Y-%m-%d") + return calculate_retention_end_datetime(start_date, retention_years).strftime( + "%Y-%m-%d" + ) ResearchDriveName = Annotated[str, AfterValidator(validate_resdrive_name)] diff --git a/src/service/activescale.py b/src/service/activescale.py index 6b3670b..068154a 100644 --- a/src/service/activescale.py +++ b/src/service/activescale.py @@ -11,6 +11,7 @@ import threading import time from contextlib import contextmanager +from datetime import datetime, timezone from pathlib import Path from typing import Any, Generator, cast @@ -583,6 +584,68 @@ def verify_uploaded_part_size( return False +def set_object_retention( + client: S3Client, + bucket_name: str, + file_key: str, + retain_until: datetime, +) -> bool: + """Apply an S3 object lock retention policy in COMPLIANCE mode to an object. + + Requires the bucket to have been created with ``ObjectLockEnabledForBucket=True``. + In COMPLIANCE mode the retention date cannot be shortened or removed — not even + by an admin — until *retain_until* has passed. + + Args: + client: An initialized S3 client. + bucket_name: Name of the S3 bucket. + file_key: Object key to protect. + retain_until: UTC-aware datetime after which the object may be deleted. + + Returns: + True if the retention was set successfully, False otherwise. + """ + if retain_until.tzinfo is None: + retain_until = retain_until.replace(tzinfo=timezone.utc) + + try: + client.put_object_retention( + Bucket=bucket_name, + Key=file_key, + Retention={ + "Mode": "COMPLIANCE", + "RetainUntilDate": retain_until, + }, + ) + _log_event( + logging.INFO, + "s3.object.retention.set", + file_key=file_key, + bucket_name=bucket_name, + retain_until=retain_until.isoformat(), + ) + return True + except ClientError as e: + _log_client_error( + "s3.object.retention.client_error", + e, + file_key=file_key, + bucket_name=bucket_name, + ) + return False + except EndpointConnectionError: + _log_endpoint_connection_error(file_key=file_key, bucket_name=bucket_name) + return False + except (BotoCoreError, OSError, ValueError, TypeError) as e: + _log_unexpected_error( + "s3.object.retention.unexpected_error", + e, + file_key=file_key, + bucket_name=bucket_name, + ) + return False + + def create_bucket( client: S3Client, bucket_name: str, diff --git a/src/workers/submission_worker.py b/src/workers/submission_worker.py index 533bc77..4ed150d 100644 --- a/src/workers/submission_worker.py +++ b/src/workers/submission_worker.py @@ -5,7 +5,7 @@ import json import logging import shutil -from datetime import datetime +from datetime import datetime, timedelta, timezone from pathlib import Path from typing import Any @@ -13,7 +13,7 @@ from api.dependencies import engine from config import get_settings -from models.common import calculate_retention_end_date +from models.common import calculate_retention_end_date, calculate_retention_end_datetime from models.submission import ArchiveJobStage, ArchiveSubmission from packaging.archive_chunks import ( ArchivePartInfo, @@ -26,6 +26,7 @@ from service.activescale import ( get_activescale_client_context, object_exists, + set_object_retention, upload_file, verify_uploaded_part_size, ) @@ -110,12 +111,28 @@ def _upload_chunked_archive_parts( # pylint: disable=too-many-arguments archive_parts_dir: Path, archive_parts: list[ArchivePartInfo], timeout_seconds: int, + retain_until: datetime | None = None, ) -> tuple[bool, list[str]]: """Upload chunked archive part files with resume support. A part key is considered already uploaded only if: - it appears in persisted submission state, and - the key currently exists in object storage. + + Args: + session: Database session for persisting progress + submission: ArchiveSubmission record being processed + client: An initialized S3 client + bucket_name: Name of the S3 bucket + object_prefix: S3 key prefix for all parts in this submission + archive_parts_dir: Local directory containing the part files + archive_parts: List of ArchivePartInfo for all parts being uploaded + timeout_seconds: Timeout for each individual part upload attempt + retain_until: Optional datetime to set for object retention + (object lock COMPLIANCE mode). If None, retention will not be set. + + Returns: + Tuple of (overall upload success, list of uploaded part keys) """ part_files = sorted(archive_parts_dir.glob("*.tar.gz.part-*")) uploaded_keys = parse_part_keys_json(submission.archive_part_keys_json) @@ -175,6 +192,17 @@ def _upload_chunked_archive_parts( # pylint: disable=too-many-arguments part_key=part_key, ) + if retain_until is not None: + if not set_object_retention(client, bucket_name, part_key, retain_until): + log_event( + logging.ERROR, + "crate.upload.part.retention_failed", + submission_id=submission.id, + drive_name=submission.drive_name, + part_key=part_key, + ) + return False, uploaded_keys + return True, uploaded_keys @@ -467,6 +495,30 @@ def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,to elapsed_ms=elapsed_ms(started_at), ) + # Compute the object retention date once for all objects in this job. + retain_until: datetime | None = None + if settings.activescale_enable_object_retention: + now_utc = datetime.now(tz=timezone.utc) + if settings.activescale_retention_override_days is not None: + retain_until = now_utc + timedelta( + days=settings.activescale_retention_override_days + ) + else: + retention_years = ( + submission.retention_period_years + or settings.activescale_default_retention_years + ) + retain_until = calculate_retention_end_datetime( + now_utc, retention_years + ) + log_event( + logging.INFO, + "crate.upload.retention.computed", + submission_id=submission_id, + drive_name=drive_name, + retain_until=retain_until.isoformat(), + ) + with get_activescale_client_context() as client: bucket_name = settings.activescale_bucket_name @@ -479,6 +531,7 @@ def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,to archive_parts_dir=archive_parts_dir, archive_parts=chunk_result.parts, timeout_seconds=settings.activescale_upload_timeout, + retain_until=retain_until, ) if upload_success: @@ -536,6 +589,19 @@ def generate_ro_crate( # pylint: disable=too-many-locals,too-many-statements,to session.add(submission) session.commit() + if upload_success and retain_until is not None: + if not set_object_retention( + client, bucket_name, file_key, retain_until + ): + log_event( + logging.ERROR, + "crate.upload.manifest.retention_failed", + submission_id=submission_id, + drive_name=drive_name, + file_key=file_key, + ) + upload_success = False + # Transition: uploading → cleanup previous_stage = submission.stage submission.stage = ArchiveJobStage.CLEANUP diff --git a/tests/test_activescale.py b/tests/test_activescale.py index a4d6537..0444e92 100644 --- a/tests/test_activescale.py +++ b/tests/test_activescale.py @@ -2,13 +2,12 @@ from __future__ import annotations +from datetime import datetime, timezone from unittest.mock import MagicMock -import pytest from botocore.exceptions import BotoCoreError, ClientError, EndpointConnectionError -from service.activescale import verify_uploaded_part_size - +from service.activescale import set_object_retention, verify_uploaded_part_size def _make_client_error(code: str) -> ClientError: return ClientError( @@ -66,3 +65,78 @@ def test_zero_byte_object(self) -> None: client.head_object.return_value = {"ContentLength": 0} assert verify_uploaded_part_size(client, "bucket", "key/empty", 0) is True + + +class TestSetObjectRetention: + _RETAIN_UNTIL = datetime(2032, 1, 1, tzinfo=timezone.utc) + + def _make_client_error(self, code: str) -> ClientError: + return ClientError( + {"Error": {"Code": code, "Message": "test error"}}, "PutObjectRetention" + ) + + def test_returns_true_on_success(self) -> None: + client = MagicMock() + client.put_object_retention.return_value = {} + + assert ( + set_object_retention(client, "bucket", "key/part-00001", self._RETAIN_UNTIL) + is True + ) + client.put_object_retention.assert_called_once_with( + Bucket="bucket", + Key="key/part-00001", + Retention={"Mode": "COMPLIANCE", "RetainUntilDate": self._RETAIN_UNTIL}, + ) + + def test_naive_datetime_is_made_utc(self) -> None: + client = MagicMock() + client.put_object_retention.return_value = {} + naive = datetime(2032, 1, 1) # no tzinfo + + assert set_object_retention(client, "bucket", "key/part-00001", naive) is True + _, kwargs = client.put_object_retention.call_args + called_date = kwargs["Retention"]["RetainUntilDate"] + assert called_date.tzinfo is not None + + def test_returns_false_on_client_error(self) -> None: + client = MagicMock() + client.put_object_retention.side_effect = self._make_client_error( + "AccessDenied" + ) + + assert ( + set_object_retention(client, "bucket", "key/part-00001", self._RETAIN_UNTIL) + is False + ) + + def test_returns_false_when_object_lock_not_enabled(self) -> None: + client = MagicMock() + client.put_object_retention.side_effect = self._make_client_error( + "InvalidRequest" + ) + + assert ( + set_object_retention(client, "bucket", "key/part-00001", self._RETAIN_UNTIL) + is False + ) + + def test_returns_false_on_endpoint_connection_error(self) -> None: + client = MagicMock() + client.put_object_retention.side_effect = EndpointConnectionError( + endpoint_url="https://example.com" + ) + + assert ( + set_object_retention(client, "bucket", "key/part-00001", self._RETAIN_UNTIL) + is False + ) + + def test_returns_false_on_botocore_error(self) -> None: + client = MagicMock() + client.put_object_retention.side_effect = BotoCoreError() + + assert ( + set_object_retention(client, "bucket", "key/part-00001", self._RETAIN_UNTIL) + is False + ) diff --git a/tests/test_chunked_upload.py b/tests/test_chunked_upload.py index e41d6bc..21581d6 100644 --- a/tests/test_chunked_upload.py +++ b/tests/test_chunked_upload.py @@ -218,3 +218,119 @@ def capture_size_check(_client, _bucket: str, key: str, expected_size: int) -> b assert success is True assert len(size_check_calls) == 1 assert size_check_calls[0] == (part_key, manifest_size) + + +def test_upload_chunked_parts_sets_retention_when_provided( + tmp_path: Path, + session: Session, + monkeypatch, +) -> None: + """When retain_until is supplied, set_object_retention is called for each part.""" + from datetime import datetime, timezone + + archive_parts_dir = tmp_path / "parts" + archive_parts_dir.mkdir(parents=True, exist_ok=True) + part = archive_parts_dir / "drive.tar.gz.part-00001" + part.write_bytes(b"hello archive") + + prefix = "drive/" + part_key = f"{prefix}{part.name}" + retain_until = datetime(2032, 6, 1, tzinfo=timezone.utc) + + submission = _create_submission(session, drive_name="resmed202200024-testing") + + retention_calls: list[tuple] = [] + + def capture_retention(_client, _bucket: str, key: str, date: datetime) -> bool: + retention_calls.append((key, date)) + return True + + monkeypatch.setattr( + "workers.submission_worker.object_exists", lambda *_a, **_k: (False, None) + ) + monkeypatch.setattr("workers.submission_worker.upload_file", lambda *_a, **_k: True) + monkeypatch.setattr( + "workers.submission_worker.verify_uploaded_part_size", lambda *_a, **_k: True + ) + monkeypatch.setattr( + "workers.submission_worker.set_object_retention", capture_retention + ) + + success, _ = _upload_chunked_archive_parts( + session=session, + submission=submission, + client=object(), + bucket_name="bucket", + object_prefix=prefix, + archive_parts_dir=archive_parts_dir, + archive_parts=[ + ArchivePartInfo( + index=1, + file_name=part.name, + size_bytes=len(b"hello archive"), + sha256="a", + ), + ], + timeout_seconds=60, + retain_until=retain_until, + ) + + assert success is True + assert len(retention_calls) == 1 + assert retention_calls[0] == (part_key, retain_until) + + +def test_upload_chunked_parts_fails_on_retention_error( + tmp_path: Path, + session: Session, + monkeypatch, +) -> None: + """If set_object_retention fails the job aborts and the part key is not persisted.""" + from datetime import datetime, timezone + + archive_parts_dir = tmp_path / "parts" + archive_parts_dir.mkdir(parents=True, exist_ok=True) + part = archive_parts_dir / "drive.tar.gz.part-00001" + part.write_bytes(b"hello archive") + + prefix = "drive/" + part_key = f"{prefix}{part.name}" + retain_until = datetime(2032, 6, 1, tzinfo=timezone.utc) + + submission = _create_submission(session, drive_name="resmed202200024-testing") + + monkeypatch.setattr( + "workers.submission_worker.object_exists", lambda *_a, **_k: (False, None) + ) + monkeypatch.setattr("workers.submission_worker.upload_file", lambda *_a, **_k: True) + monkeypatch.setattr( + "workers.submission_worker.verify_uploaded_part_size", lambda *_a, **_k: True + ) + monkeypatch.setattr( + "workers.submission_worker.set_object_retention", lambda *_a, **_k: False + ) + + success, result_keys = _upload_chunked_archive_parts( + session=session, + submission=submission, + client=object(), + bucket_name="bucket", + object_prefix=prefix, + archive_parts_dir=archive_parts_dir, + archive_parts=[ + ArchivePartInfo( + index=1, + file_name=part.name, + size_bytes=len(b"hello archive"), + sha256="a", + ), + ], + timeout_seconds=60, + retain_until=retain_until, + ) + + assert success is False + # Part was uploaded and size-verified but retention failed — + # it should still be recorded as uploaded so a retry skips re-uploading it + # but the job overall is failed. + assert part_key in result_keys diff --git a/tests/test_chunked_workflow_integration.py b/tests/test_chunked_workflow_integration.py index cc12bf6..d14ea1d 100644 --- a/tests/test_chunked_workflow_integration.py +++ b/tests/test_chunked_workflow_integration.py @@ -106,6 +106,9 @@ def test_generate_ro_crate_chunked_success_and_manifest_integrity( archive_chunk_manifest_file_name="archive-manifest.json", activescale_upload_timeout=60, activescale_bucket_name="research-archive-test", + activescale_enable_object_retention=False, + activescale_default_retention_years=6, + activescale_retention_override_days=None, ) monkeypatch.setattr("workers.submission_worker.get_settings", lambda: settings) @@ -197,6 +200,9 @@ def test_generate_ro_crate_resumes_after_interrupted_part_upload( archive_chunk_manifest_file_name="archive-manifest.json", activescale_upload_timeout=60, activescale_bucket_name="research-archive-test", + activescale_enable_object_retention=False, + activescale_default_retention_years=6, + activescale_retention_override_days=None, ) monkeypatch.setattr("workers.submission_worker.get_settings", lambda: settings) From 74ad17f9df53c2332b68a3c3426c87418eb454f9 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Wed, 24 Jun 2026 16:00:48 +1200 Subject: [PATCH 11/13] add poe dev task --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index e2a8774..3407b6a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -80,3 +80,4 @@ fix = [ "black-fix" ] coverage-report = "python -m coverage report -m" +dev = "fastapi dev src/api/main.py" From 7980bfe8cdc2af27c101e560bcbf613b55ebe6d4 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Tue, 28 Jul 2026 13:09:06 +1200 Subject: [PATCH 12/13] comment and env file cleanup --- modes/.env.development | 1 - modes/.env.production | 2 +- src/service/activescale.py | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modes/.env.development b/modes/.env.development index 0b2ae2b..629a7fb 100644 --- a/modes/.env.development +++ b/modes/.env.development @@ -1,6 +1,5 @@ VITE_API_BASE_URL=http://localhost:8000 CORS_ALLOW_HOST='["http://localhost:5173"]' -# SMB_DRIVE_BASE_PATH=//files.auckland.ac.nz/research # Unifiles SMB_DRIVE_BASE_PATH=//test-2.d02pvaststg01.uoa.auckland.ac.nz SMB_LINUX_MOUNT_BASE_PATH=/mnt ACTIVESCALE_HOSTNAME=asi.s3.prod.nz diff --git a/modes/.env.production b/modes/.env.production index fb6d45b..4cc455e 100644 --- a/modes/.env.production +++ b/modes/.env.production @@ -1,5 +1,5 @@ CORS_ALLOW_HOST='["https://uoa-eresearch.github.io"]' -SMB_DRIVE_BASE_PATH=//files.auckland.ac.nz/research +SMB_DRIVE_BASE_PATH= SMB_LINUX_MOUNT_BASE_PATH=/mnt ACTIVESCALE_HOSTNAME=asi.s3.prod.nz ACTIVESCALE_REGION=us-east-1 diff --git a/src/service/activescale.py b/src/service/activescale.py index 068154a..f51385c 100644 --- a/src/service/activescale.py +++ b/src/service/activescale.py @@ -114,6 +114,7 @@ def _create_activescale_session() -> boto3.Session: ) if settings.log_level.upper() == "DEBUG": + # Enable detailed logging for boto3 when log level is DEBUG boto3.set_stream_logger("") session = boto3.Session( From 64c76969843017426f0ba07ecc39c81c58a31dd2 Mon Sep 17 00:00:00 2001 From: rosemcc Date: Tue, 28 Jul 2026 13:14:19 +1200 Subject: [PATCH 13/13] updated todo list --- TODO.md | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/TODO.md b/TODO.md index c15fed7..6035095 100644 --- a/TODO.md +++ b/TODO.md @@ -6,12 +6,23 @@ ## Quality and Validation +- [x] Improved object and Tar integrity checks (e.g. validate checksums of uploaded parts, verify manifest integrity, and ensure reassembled archive matches original input). +- [ ] Add minimum metadata validation for the archive job (required fields are documented in the Wiki). This should be checked upfront in the API request validation so a job fails fast if there is missing metadata. Allow force flag to override this check if needed. +- [ ] Validate generated RO-Crate output against the profile (follows on from the previous item). This should be done after the RO-Crate is generated, but before the archive is uploaded. If the RO-Crate fails validation, the job should fail and log an error message indicating what was invalid. +- [ ] Ensure temp files are cleaned up properly from disk after a job completes or fails. Consider sdelete for permanent deletion of data. - [ ] Improve end-to-end tests that cover submission -> manifest -> RO-Crate build -> upload flow. -- [ ] Validate generated RO-Crate output against the profile. -- [ ] Improved object and Tar integrity checks (e.g. validate checksums of uploaded parts, verify manifest integrity, and ensure reassembled archive matches original input). - [ ] Question: should custom s3 metadata be added on every uploaded object/prat of the archive? Currently just the archive manifest object has the metadata added. ## Features and Enhancements +- [x] Set retention policies on the stored archive objects to prevent accidental deletion, and ensure long-term preservation of the archived data. +- [ ] Convert this project to use uv and replace linting etc with Ruff +- [ ] The ProjectDB now stores data retention and classification metadata (Storage Properties table). The archive submission workflow should be updated to use this metadata from ProjectDB instead of requiring it to be provided in the API request. If values are provided in the request, they should override the ProjectDB values (but not alter the values in the ProjectDB). If the retention and classification are incorect in the projectDB they can be updated via the ProjectDB API, but driveoff s not responsible for updating ProjectDB values. +- [ ] Add workflow for deleting the original drive data after successful archive. Key steps would be: flagging the source data as ready for deletion, running a separate cleanup job that verifies the archive integrity, and verifies the object exists before deleting, and handling any edge cases (e.g. what if the archive is corrupted?). It would also need to retain a copy of the archive manifest, and location of the stored archive, in the research drive (the drives/views/shares themselves will not be deleted). This may be a completely separate workflow from the archiving process, OR could be triggered from the archive submission API endpoint by adding an additional parameter to indicate whether deletion should be performed after archiving - design decision needed. - [ ] Notifications module to send slack messages to admins when jobs complete or fail -- [ ] Logging to a durable store (e.g. file, database, or logging service) instead of just stdout for better traceability and debugging. -- [ ] Add workflow for deleting the original drive data after successful archive. Key steps would be: flagging the source data as ready for deletion, running a separate cleanup job that verifies the archive integrity, and verifies the object exists before deleting, and handling any edge cases (e.g. what if the archive is corrupted?). It would also need to retain a copy of the archive manifest, and location of the stored archive, in the research drive (the drives/views/shares themselves will not be deleted). This may be a separate workflow from the archiving process, but could be triggered from the same API endpoint by adding an additional parameter to indicate whether deletion should be performed after archiving. + +## Infrastructure and Deployment +- [ ] Add scripts/playbook for setting up the VM this will run on. Initial thought is to use Ansible for configuration management to set up the application and its dependencies. This will help automate the deployment process and ensure consistency across environments. Key steps would include: set squid proxy variables, configure git, clone repo, install Python, install Poetry (or uv if we switch), install dependencies, setup api keys file for driveoff (expects allowed keys to be in api_keys.json file), set up environment variables for configuration, and setup a process manager (e.g. systemd or supervisor) to run the application as a service and ensure it restarts on failure (for initial phase we may just run the fastapi manually though). +- [ ] Setup autofs for auto mount and unmount of drives (if we will use linux) +- [ ] Setup secrets management service - investigate options (Barbican, AWS Parameter Store, Hashicorp Vault, etc.). +- [ ] Following on from the above item, set up a better solution for storing and managing API keys for the driveoff service. Currently, the allowed keys are stored in a JSON file, but a more secure and manageable solution should be implemented (e.g. using a secrets management service or encrypted storage). +- [ ] Setup a monitoring tool and external log aggregation - investigate options for logging to a durable store (e.g. file, database, or logging service) instead of just stdout for better traceability and debugging.