diff --git a/Cargo.lock b/Cargo.lock index 7574e8aa..666baa47 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2822,7 +2822,6 @@ version = "0.2.10" dependencies = [ "anyhow", "async-compression", - "base64", "bytes", "futures-util", "http-body-util", @@ -2952,7 +2951,6 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", - "base64", "bigtable_rs", "blake3", "bytes", diff --git a/clients/CHANGELOG.md b/clients/CHANGELOG.md index 8f0c8009..6246f8ee 100644 --- a/clients/CHANGELOG.md +++ b/clients/CHANGELOG.md @@ -143,7 +143,6 @@ ### Documentation 📚 - Fix and expand documentation across the workspace by @jan-auer in [#521](https://github.com/getsentry/objectstore/pull/521) -- Fix multipart endpoint paths and client examples by @jan-auer in [#505](https://github.com/getsentry/objectstore/pull/505) ### Internal Changes 🔧 @@ -167,8 +166,6 @@ ### New Features ✨ -- (py-client) Add Multipart Upload API by @lcian in [#476](https://github.com/getsentry/objectstore/pull/476) -- (rust-client) Add Multipart Upload API by @lcian in [#468](https://github.com/getsentry/objectstore/pull/468) ## 0.1.9 diff --git a/clients/python/README.md b/clients/python/README.md index 924eb82a..ef04fa82 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -116,66 +116,6 @@ Arbitrary key-value pairs can be attached to objects and retrieved on download. session.put(b"payload", metadata={"source": "upload-service"}) ``` -### Multipart Upload API - -For large objects, use multipart uploads to upload parts independently and then -assemble them into a final object. - -**Important:** unlike single-object uploads, multipart uploads do **not** auto-compress. -`compression` on `initiate_multipart_upload` behaves like `precompressed` above: it only -records the algorithm, and the caller must pre-compress each part accordingly. - -```python -from concurrent.futures import ThreadPoolExecutor - -import zstandard - -from objectstore_client.multipart import MultipartCompleteError - -upload = session.initiate_multipart_upload( - key="my-large-object", - compression="zstd", - metadata={"source": "upload-service"}, -) - -compressor = zstandard.ZstdCompressor() -chunks = [b"part1", b"part2", b"part3", b"part4"] - -def upload_part(part_number: int, data: bytes): - compressed = compressor.compress(data) - return upload.put_part( - compressed, part_number=part_number, content_length=len(compressed) - ) - -with ThreadPoolExecutor(max_workers=4) as executor: - futures = [ - executor.submit(upload_part, i + 1, chunk) - for i, chunk in enumerate(chunks) - ] - parts = [f.result() for f in futures] - -try: - key = upload.complete(parts) -except MultipartCompleteError: - upload.abort() - raise -``` - -To resume an in-progress multipart upload after a process restart, persist the -`key` and `upload_id`, then reconstruct the upload handle later: - -```python -saved_key = upload.key -saved_upload_id = upload.upload_id - -resumed = session.resume_multipart_upload(saved_key, saved_upload_id) -existing_parts = resumed.list_parts() - -# Upload missing parts... - -key = resumed.complete(new_parts + existing_parts) -``` - ### Authentication If your Objectstore instance enforces authorization, you must configure authentication diff --git a/clients/python/docs/objectstore_client.rst b/clients/python/docs/objectstore_client.rst index 7d53885b..061b2131 100644 --- a/clients/python/docs/objectstore_client.rst +++ b/clients/python/docs/objectstore_client.rst @@ -49,14 +49,6 @@ objectstore\_client.metrics module :show-inheritance: :undoc-members: -objectstore\_client.multipart module ------------------------------------- - -.. automodule:: objectstore_client.multipart - :members: - :show-inheritance: - :undoc-members: - objectstore\_client.presign module ---------------------------------- diff --git a/clients/python/src/objectstore_client/client.py b/clients/python/src/objectstore_client/client.py index d4f82b3f..c86c4184 100644 --- a/clients/python/src/objectstore_client/client.py +++ b/clients/python/src/objectstore_client/client.py @@ -41,7 +41,6 @@ NoOpMetricsBackend, measure_storage_operation, ) -from objectstore_client.multipart import MultipartUpload from objectstore_client.scope import Scope from objectstore_client.tracing import storage_span @@ -94,7 +93,7 @@ def __init__( "usecase name must contain only ASCII letters, digits, '_' or '-'" ) self._name = name - + if resumable_threshold_bytes != "none" and ( type(resumable_threshold_bytes) is not int or resumable_threshold_bytes < 0 ): @@ -400,25 +399,6 @@ def _make_url(self, key: str | None, full: bool = False) -> str: return f"{self._base_url()}{path}" return path - def _make_multipart_url( - self, - action: str | None, - key: str | None, - query: str | None = None, - ) -> str: - if action == "parts": - resource = "objects:multipart:parts" - elif action == "complete": - resource = "objects:multipart:complete" - else: - resource = "objects:multipart" - - relative_path = f"/v1/{resource}/{self._usecase.name}/{self._scope}/{key or ''}" - path = utils.encode_path(self._base_path.rstrip("/") + relative_path) - if query: - return f"{path}?{query}" - return path - def put( self, contents: bytes | IO[bytes], @@ -1003,73 +983,3 @@ def _create_upload( def _resume_upload(self, key: str, token: str) -> _resumable.ResumableUpload: """Reconstruct a resumable upload handle, without contacting the server.""" return _resumable.ResumableUpload(self, key, token) - - def initiate_multipart_upload( - self, - *, - key: str | None = None, - compression: Compression | Literal["none"] | None = None, - content_type: str | None = None, - metadata: dict[str, str] | None = None, - expiration_policy: ExpirationPolicy | None = None, - origin: str | None = None, - filename: str | None = None, - ) -> MultipartUpload: - """ - Initiates a multipart upload. - - Returns a :class:`~objectstore_client.multipart.MultipartUpload` handle - that can be used to upload parts, list parts, complete, or abort. - - **Important:** unlike :meth:`put`, the ``compression`` parameter only - records the compression algorithm in the object's metadata. - The caller is responsible for compressing each part in accordance with the - chosen algorithm before passing it to - :meth:`~objectstore_client.multipart.MultipartUpload.upload_part`. - """ - if compression and compression not in ("none", "zstd"): - raise ValueError(f"Invalid compression: {compression}") - - headers = self._metadata_headers( - compression=compression or self._usecase._compression, - content_type=content_type, - metadata=metadata, - expiration_policy=expiration_policy, - origin=origin, - filename=filename, - ) - - if key == "": - key = None - - with ( - storage_span( - "multipart.initiate", self._usecase, self._scope, key=key - ) as span, - measure_storage_operation( - self._metrics_backend, "multipart.initiate", self._usecase.name - ), - ): - response = self._pool.request( - "POST" if not key else "PUT", - self._make_multipart_url(None, key), - headers=headers, - preload_content=True, - decode_content=True, - ) - raise_for_status(response) - res = response.json() - # Set after the response, since the key may be server-generated. - span.set_attribute("objectstore.key", res["key"]) - span.set_attribute("objectstore.upload_id", res["upload_id"]) - return MultipartUpload(self, res["key"], res["upload_id"]) - - def resume_multipart_upload(self, key: str, upload_id: str) -> MultipartUpload: - """ - Reconstructs a multipart upload handle. - - This does not make any network calls. - Use it to resume an upload after a process restart or to - continue an upload started elsewhere. - """ - return MultipartUpload(self, key, upload_id) diff --git a/clients/python/src/objectstore_client/multipart.py b/clients/python/src/objectstore_client/multipart.py deleted file mode 100644 index da00f156..00000000 --- a/clients/python/src/objectstore_client/multipart.py +++ /dev/null @@ -1,284 +0,0 @@ -from __future__ import annotations - -import base64 -import json -from collections.abc import Sequence -from dataclasses import dataclass -from datetime import datetime -from io import BytesIO -from typing import IO, TYPE_CHECKING -from urllib.parse import urlencode - -from objectstore_client.errors import RequestError, raise_for_status -from objectstore_client.metrics import measure_storage_operation -from objectstore_client.tracing import storage_span - -if TYPE_CHECKING: - from objectstore_client.client import Session - - -@dataclass -class CompletePart: - """A reference to an uploaded part, used when completing a multipart upload.""" - - part_number: int - etag: str - - -@dataclass -class PartInfo: - """Information about an uploaded part""" - - part_number: int - etag: str - last_modified: datetime - size: int - - -class MultipartCompleteError(RequestError): - """Error returned as part of a multipart:complete response's body.""" - - def __init__(self, code: str, message: str): - super().__init__( - f"Multipart upload completion failed ({code}): {message}", - status=200, - response=message, - ) - self.code = code - - -class MultipartUpload: - """ - Handle for an in-progress multipart upload. - - Create via :meth:`~objectstore_client.client.Session.initiate_multipart_upload` or - :meth:`~objectstore_client.client.Session.resume_multipart_upload`. - """ - - def __init__(self, session: Session, key: str, upload_id: str): - self._session = session - self._key = key - self._upload_id = upload_id - - @property - def key(self) -> str: - return self._key - - @property - def upload_id(self) -> str: - return self._upload_id - - def put_part( - self, - contents: bytes | IO[bytes], - *, - part_number: int, - content_length: int, - content_md5: bytes | None = None, - ) -> CompletePart: - """ - Uploads a single part. - - IMPORTANT: Unlike - :meth:`~objectstore_client.client.Session.put`, - this does **not** automatically compress `contents`. - The caller must pre-compress each part according to the - compression set as part of the metadata when initiating - the upload. - - Args: - contents: The part data. If this upload was initiated - with compression, this must be pre-compressed. - part_number: 1-indexed part number. - content_length: The length in bytes of the payload - being uploaded. If this upload was initiated with - compression, this must be the post-compression - length. - content_md5: Optional raw MD5 digest of `contents`. - """ - if isinstance(contents, bytes): - if len(contents) != content_length: - raise ValueError( - "content_length must match the size of the provided payload" - ) - body: bytes | IO[bytes] = BytesIO(contents) - else: - body = contents - - if content_md5 is not None and len(content_md5) != 16: - raise ValueError("content_md5 must be exactly 16 bytes") - - headers = self._session._make_headers() - headers["Content-Length"] = str(content_length) - - if content_md5 is not None: - headers["Content-MD5"] = base64.b64encode(content_md5).decode("ascii") - - query = urlencode( - {"upload_id": self._upload_id, "part_number": str(part_number)} - ) - url = self._session._make_multipart_url("parts", self._key, query) - - with ( - storage_span( - "multipart.put_part", - self._session._usecase, - self._session._scope, - key=self._key, - upload_id=self._upload_id, - part_number=part_number, - size=content_length, - ), - measure_storage_operation( - self._session._metrics_backend, - "multipart.put_part", - self._session._usecase.name, - ) as metric_emitter, - ): - response = self._session._pool.request( - "PUT", - url, - body=body, - headers=headers, - preload_content=True, - decode_content=True, - ) - raise_for_status(response) - res = response.json() - metric_emitter.record_size(content_length) - return CompletePart(part_number=part_number, etag=res["etag"]) - - def list_parts(self) -> list[PartInfo]: - """Lists all uploaded parts.""" - all_parts: list[PartInfo] = [] - marker: int | None = None - - with storage_span( - "multipart.list_parts", - self._session._usecase, - self._session._scope, - key=self._key, - upload_id=self._upload_id, - ) as span: - while True: - params: dict[str, str] = {"upload_id": self._upload_id} - if marker is not None: - params["part_number_marker"] = str(marker) - - query = urlencode(params) - url = self._session._make_multipart_url("parts", self._key, query) - headers = self._session._make_headers() - - response = self._session._pool.request( - "GET", - url, - headers=headers, - preload_content=True, - ) - raise_for_status(response) - data = response.json() - - for p in data["parts"]: - all_parts.append( - PartInfo( - part_number=p["part_number"], - etag=p["etag"], - last_modified=datetime.fromisoformat(p["last_modified"]), - size=p["size"], - ) - ) - - if not data["is_truncated"]: - span.set_attribute("objectstore.part_count", len(all_parts)) - return all_parts - - marker = data.get("next_part_number_marker") - if marker is None: - raise RequestError( - "Server returned is_truncated=true but no " - "next_part_number_marker", - status=200, - response=str(data), - ) - - def abort(self) -> None: - """Aborts this multipart upload, cleaning up server-side state.""" - with storage_span( - "multipart.abort", - self._session._usecase, - self._session._scope, - key=self._key, - upload_id=self._upload_id, - ): - query = urlencode({"upload_id": self._upload_id}) - url = self._session._make_multipart_url(None, self._key, query) - headers = self._session._make_headers() - - response = self._session._pool.request( - "DELETE", - url, - headers=headers, - ) - raise_for_status(response) - - def complete(self, parts: Sequence[CompletePart | PartInfo]) -> str: - """Completes the multipart upload, assembling all parts into the final object. - - Returns the final object key. - - Raises :class:`MultipartCompleteError` if the server reports an error - during assembly, or :class:`RequestError` if the server returns a non-2XX - response. - """ - query = urlencode({"upload_id": self._upload_id}) - url = self._session._make_multipart_url("complete", self._key, query) - headers = self._session._make_headers() - headers["Content-Type"] = "application/json" - - sorted_parts = sorted(parts, key=lambda p: p.part_number) - request_body = json.dumps( - { - "parts": [ - {"part_number": p.part_number, "etag": p.etag} for p in sorted_parts - ] - } - ).encode("utf-8") - - with ( - storage_span( - "multipart.complete", - self._session._usecase, - self._session._scope, - key=self._key, - upload_id=self._upload_id, - part_count=len(sorted_parts), - ), - measure_storage_operation( - self._session._metrics_backend, - "multipart.complete", - self._session._usecase.name, - ), - ): - response = self._session._pool.request( - "POST", - url, - body=request_body, - headers=headers, - preload_content=True, - decode_content=True, - ) - raise_for_status(response) - - raw = (response.data or b"").decode("utf-8").strip() - try: - data = json.loads(raw) - except json.JSONDecodeError: - raise ValueError("Failed to parse multipart complete response") - - if "error" in data: - raise MultipartCompleteError( - code=data["error"]["code"], - message=data["error"]["message"], - ) - - return data["key"] diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 1f08b373..b82bac34 100644 --- a/clients/python/tests/test_e2e.py +++ b/clients/python/tests/test_e2e.py @@ -34,7 +34,6 @@ from objectstore_client.auth import Permission, SecretKey from objectstore_client.errors import RequestError from objectstore_client.metadata import TimeToLive -from objectstore_client.multipart import CompletePart, MultipartCompleteError from objectstore_client.scope import Scope TEST_EDDSA_KID: str = "test_kid" @@ -642,314 +641,6 @@ def test_connect_timeout() -> None: session.put(b"test data", compress="zstd") -def test_multipart_full_cycle_uncompressed(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-uncompressed") - assert upload.key == "mp-uncompressed" - assert upload.upload_id - - part1 = upload.put_part(b"hello ", part_number=1, content_length=6) - part2 = upload.put_part(b"world!", part_number=2, content_length=6) - - final_key = upload.complete([part1, part2]) - assert final_key == "mp-uncompressed" - - retrieved = session.get(final_key, decompress=False) - assert retrieved is not None - assert retrieved.payload.read() == b"hello world!" - - -def test_multipart_full_cycle_compressed(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload( - key="mp-compressed", - compression="zstd", - ) - - cctx = zstandard.ZstdCompressor() - compressed_part1 = cctx.compress(b"hello ") - compressed_part2 = cctx.compress(b"world!") - - part1 = upload.put_part( - compressed_part1, part_number=1, content_length=len(compressed_part1) - ) - part2 = upload.put_part( - compressed_part2, part_number=2, content_length=len(compressed_part2) - ) - - final_key = upload.complete([part1, part2]) - - # Verify raw compressed round-trip - retrieved = session.get(final_key, decompress=False) - assert retrieved is not None - assert retrieved.metadata.compression == "zstd" - raw = retrieved.payload.read() - assert raw == compressed_part1 + compressed_part2 - - # Verify transparent decompression - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.metadata.compression is None - assert retrieved.payload.read() == b"hello world!" - - -def test_multipart_streaming_part_upload_uncompressed(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-streaming-uncompressed") - - part1_payload = b"hello " - part2_payload = b"world!" - part1 = upload.put_part( - UnrewindableStream(part1_payload), - part_number=1, - content_length=len(part1_payload), - ) - part2 = upload.put_part( - UnrewindableStream(part2_payload), - part_number=2, - content_length=len(part2_payload), - ) - - final_key = upload.complete([part1, part2]) - - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.payload.read() == b"hello world!" - - -def test_multipart_streaming_part_upload_compressed(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload( - key="mp-streaming-compressed", - compression="zstd", - ) - - cctx = zstandard.ZstdCompressor() - compressed_part1 = cctx.compress(b"hello ") - compressed_part2 = cctx.compress(b"world!") - - part1 = upload.put_part( - UnrewindableStream(compressed_part1), - part_number=1, - content_length=len(compressed_part1), - ) - part2 = upload.put_part( - UnrewindableStream(compressed_part2), - part_number=2, - content_length=len(compressed_part2), - ) - - final_key = upload.complete([part1, part2]) - - retrieved = session.get(final_key, decompress=False) - assert retrieved is not None - assert retrieved.metadata.compression == "zstd" - assert retrieved.payload.read() == compressed_part1 + compressed_part2 - - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.metadata.compression is None - assert retrieved.payload.read() == b"hello world!" - - -def test_multipart_server_generated_key(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload() - assert upload.key - - part = upload.put_part(b"data", part_number=1, content_length=4) - final_key = upload.complete([part]) - assert final_key - - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.payload.read() == b"data" - - -def test_multipart_list_parts(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-list-parts") - - upload.put_part(b"part-two", part_number=2, content_length=8) - upload.put_part(b"part-one", part_number=1, content_length=8) - - parts = upload.list_parts() - assert len(parts) == 2 - - p1 = next(p for p in parts if p.part_number == 1) - p2 = next(p for p in parts if p.part_number == 2) - assert p1.size == 8 - assert p2.size == 8 - - upload.abort() - - -def test_multipart_abort(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-abort") - upload.put_part(b"some data", part_number=1, content_length=9) - upload.abort() - - -def test_multipart_metadata_preserved(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload( - key="mp-metadata", - content_type="text/plain", - origin="203.0.113.42", - filename="archive.tar.gz", - metadata={"my-key": "my-value"}, - ) - - part = upload.put_part(b"payload", part_number=1, content_length=7) - final_key = upload.complete([part]) - - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.metadata.content_type == "text/plain" - assert retrieved.metadata.origin == "203.0.113.42" - assert retrieved.metadata.filename == "archive.tar.gz" - assert retrieved.metadata.custom.get("my-key") == "my-value" - - -def test_multipart_complete_with_bad_etag(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-bad-etag") - upload.put_part(b"real data", part_number=1, content_length=9) - - with pytest.raises(MultipartCompleteError) as exc_info: - upload.complete([CompletePart(part_number=1, etag="bogus-etag")]) - - assert exc_info.value.code - assert exc_info.value.status == 200 - - -def test_multipart_resume(server_url: str) -> None: - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-resume") - saved_key = upload.key - saved_upload_id = upload.upload_id - - upload.put_part(b"first", part_number=1, content_length=5) - - # Simulate resuming from saved state - resumed = session.resume_multipart_upload(saved_key, saved_upload_id) - assert resumed.key == saved_key - assert resumed.upload_id == saved_upload_id - - resumed.put_part(b"second", part_number=2, content_length=6) - - existing = resumed.list_parts() - assert len(existing) == 2 - - final_key = resumed.complete(existing) - - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.payload.read() == b"firstsecond" - - -def test_multipart_concurrent_part_uploads(server_url: str) -> None: - from concurrent.futures import ThreadPoolExecutor - - client = Client(server_url, token=TestSecretKey.get()) - usecase = Usecase( - "test-usecase", - compression="none", - expiration_policy=TimeToLive(timedelta(days=1)), - ) - session = client.session(usecase, org=42, project=1337) - - upload = session.initiate_multipart_upload(key="mp-concurrent") - - chunks = [f"chunk-{i}".encode() for i in range(8)] - - def put_part(part_number: int, data: bytes) -> CompletePart: - return upload.put_part(data, part_number=part_number, content_length=len(data)) - - with ThreadPoolExecutor(max_workers=4) as executor: - futures = [ - executor.submit(put_part, i + 1, chunk) for i, chunk in enumerate(chunks) - ] - parts = [f.result() for f in futures] - - final_key = upload.complete(parts) - - retrieved = session.get(final_key) - assert retrieved is not None - assert retrieved.payload.read() == b"".join(chunks) - - def _fetch(url: str, method: str = "GET") -> tuple[int, bytes]: """Fetches ``url`` with plain urllib (no auth header), returning (status, body). diff --git a/clients/python/tests/test_multipart.py b/clients/python/tests/test_multipart.py deleted file mode 100644 index 1b3966dd..00000000 --- a/clients/python/tests/test_multipart.py +++ /dev/null @@ -1,59 +0,0 @@ -import json -from typing import Any - -import pytest -from objectstore_client import Client, Usecase -from objectstore_client.errors import RequestError -from objectstore_client.multipart import CompletePart, MultipartUpload - - -class FakeResponse: - def __init__( - self, - status: int, - *, - data: bytes = b"", - json_data: dict[str, Any] | None = None, - ) -> None: - self.status = status - self.data = data - self._json_data = json_data - - def read(self) -> bytes: - return self.data - - def json(self) -> dict[str, Any]: - if self._json_data is not None: - return self._json_data - return json.loads(self.data.decode("utf-8")) - - -def test_upload_part_validates_bytes_content_length() -> None: - client = Client("http://127.0.0.1:8888") - session = client.session(Usecase("testing"), org=1) - upload = MultipartUpload(session, "key", "upload-id") - - with pytest.raises(ValueError, match="content_length must match"): - upload.put_part(b"payload", part_number=1, content_length=1) - - -def test_multipart_complete_raises_http_errors_before_parsing( - monkeypatch: pytest.MonkeyPatch, -) -> None: - client = Client("http://127.0.0.1:8888") - session = client.session(Usecase("testing"), org=1) - upload = MultipartUpload(session, "key", "upload-id") - - monkeypatch.setattr( - session._pool, - "request", - lambda *args, **kwargs: FakeResponse( - 403, data=b'{"detail":"missing or expired auth"}' - ), - ) - - with pytest.raises(RequestError) as exc_info: - upload.complete([CompletePart(part_number=1, etag="etag")]) - - assert exc_info.value.status == 403 - assert exc_info.value.response == '{"detail":"missing or expired auth"}' diff --git a/clients/python/tests/test_tracing.py b/clients/python/tests/test_tracing.py index bc01e0f0..beff53e2 100644 --- a/clients/python/tests/test_tracing.py +++ b/clients/python/tests/test_tracing.py @@ -9,7 +9,6 @@ import sentry_sdk from objectstore_client import Client, Usecase from objectstore_client.errors import RequestError -from objectstore_client.multipart import MultipartUpload from sentry_sdk.envelope import Envelope from sentry_sdk.transport import Transport from sentry_sdk.types import Hint @@ -121,56 +120,6 @@ def _spans_by_op(spans: list[SpanJSON]) -> dict[str, SpanJSON]: {"objectstore.key": "my-key"}, id="delete", ), - pytest.param( - lambda upload: upload["session"].initiate_multipart_upload(key="my-key"), - FakeResponse(200, json_data={"key": "my-key", "upload_id": "upload-1"}), - "objectstore.multipart.initiate", - {"objectstore.key": "my-key", "objectstore.upload_id": "upload-1"}, - id="multipart.initiate", - ), - pytest.param( - lambda upload: upload["upload"].put_part( - b"part-data", part_number=1, content_length=len(b"part-data") - ), - FakeResponse(200, json_data={"etag": "etag-1"}), - "objectstore.multipart.put_part", - { - "objectstore.key": "my-key", - "objectstore.upload_id": "upload-1", - "objectstore.part_number": 1, - "objectstore.size": len(b"part-data"), - }, - id="multipart.put_part", - ), - pytest.param( - lambda upload: upload["upload"].list_parts(), - FakeResponse(200, json_data={"parts": [], "is_truncated": False}), - "objectstore.multipart.list_parts", - { - "objectstore.key": "my-key", - "objectstore.upload_id": "upload-1", - "objectstore.part_count": 0, - }, - id="multipart.list_parts", - ), - pytest.param( - lambda upload: upload["upload"].complete([]), - FakeResponse(200, data=b'{"key": "my-key"}'), - "objectstore.multipart.complete", - { - "objectstore.key": "my-key", - "objectstore.upload_id": "upload-1", - "objectstore.part_count": 0, - }, - id="multipart.complete", - ), - pytest.param( - lambda upload: upload["upload"].abort(), - FakeResponse(200), - "objectstore.multipart.abort", - {"objectstore.key": "my-key", "objectstore.upload_id": "upload-1"}, - id="multipart.abort", - ), ] @@ -186,12 +135,11 @@ def test_storage_span_attributes( client = Client("http://127.0.0.1:8888") usecase = Usecase("testing") session = client.session(usecase, org=1) - upload = MultipartUpload(session, "my-key", "upload-1") monkeypatch.setattr(session._pool, "request", lambda *args, **kwargs: response) with sentry_sdk.traces.start_span(name="test", parent_span=None): - call({"session": session, "upload": upload}) + call({"session": session}) spans = _spans_by_op(captured_spans) span = spans[expected_op] diff --git a/clients/rust/Cargo.toml b/clients/rust/Cargo.toml index 7d5e2d3a..85c3bc6b 100644 --- a/clients/rust/Cargo.toml +++ b/clients/rust/Cargo.toml @@ -15,7 +15,6 @@ features = ["resumable-upload-api"] [dependencies] async-compression = { workspace = true, features = ["tokio", "zstd"] } -base64 = { workspace = true, optional = true } bytes = { workspace = true } futures-util = { workspace = true } infer = { workspace = true } @@ -45,14 +44,8 @@ zstd = { workspace = true } [features] default = ["native-tls", "hickory-dns"] -multipart = ["dep:base64"] resumable-upload-api = [] rustls = ["reqwest/rustls"] native-tls = ["reqwest/native-tls"] hickory-dns = ["reqwest/hickory-dns"] - -[[test]] -name = "multipart" -path = "tests/multipart.rs" -required-features = ["multipart"] diff --git a/clients/rust/README.md b/clients/rust/README.md index c48651c1..3971c379 100644 --- a/clients/rust/README.md +++ b/clients/rust/README.md @@ -210,96 +210,6 @@ async fn upload_large_object(session: &Session, object: Bytes) -> Result<()> { } ``` -### Multipart Upload API - -> **Feature flag required:** Enable the `multipart` Cargo feature to use this API. -> It is not included in the default feature set. -> -> ```toml -> objectstore-client = { version = "...", features = ["multipart"] } -> ``` - -For large objects, use multipart uploads to upload parts concurrently with bounded -parallelism. - -**Important:** unlike single-object uploads, multipart uploads do **not** auto-compress. -`compression` on the initiate request behaves like `precompressed` above: it only records the -algorithm, and the caller must pre-compress each part accordingly. - -```rust,ignore -use futures_util::StreamExt as _; -use futures_util::stream; -use objectstore_client::Compression; - -let upload = session - .initiate_multipart_upload() - .key("my-large-object") - .compression(Compression::Zstd) - .send() - .await?; - -let parts: Vec<(Vec, u32)> = vec![ - (zstd::encode_all(&part1_data[..], 0)?, 1), - (zstd::encode_all(&part2_data[..], 0)?, 2), -]; - -let results: Vec<_> = stream::iter( - parts - .into_iter() - .map(|(data, part_number)| upload.put(data, part_number, None)), -) -.buffer_unordered(8) -.collect() -.await; - -let mut done = Vec::new(); -let mut errors = Vec::new(); -for result in results { - match result { - Ok(part) => done.push(part), - Err(e) => errors.push(e), - } -} - -if !errors.is_empty() { - // reupload failed parts... -} - -let key = upload.complete(done).await?; -// or -upload.abort().await?; -``` - -You can also resume an in-progress multipart upload, e.g. after a process restart. - -```rust,ignore -use futures_util::{StreamExt as _, TryStreamExt as _}; -use futures_util::stream; -use objectstore_client::CompletePart; - -let upload = session.resume_multipart_upload("my-large-object", saved_upload_id)?; - -let existing = upload.list_parts().await?; -let total_parts = 10; -let uploaded: Vec = existing.iter().map(|p| p.part_number.get()).collect(); -let missing: Vec = (1..=total_parts) - .filter(|n| !uploaded.contains(n)) - .collect(); - -let mut done: Vec<_> = stream::iter( - missing - .into_iter() - .map(|part_number| upload.put(get_part_data(part_number), part_number, None)), -) -.buffer_unordered(8) -.try_collect() -.await?; - -done.extend(existing.into_iter().map(CompletePart::from)); - -let key = upload.complete(done).await?; -``` - ### Many API The Many API allows you to enqueue multiple requests that the client can execute using Objectstore's batch endpoint, minimizing network overhead. diff --git a/clients/rust/src/client.rs b/clients/rust/src/client.rs index 64eb0cd1..5ca4032f 100644 --- a/clients/rust/src/client.rs +++ b/clients/rust/src/client.rs @@ -547,41 +547,6 @@ impl Session { url } - #[cfg(feature = "multipart")] - fn multipart_url( - &self, - suffix: Option<&'static str>, - object_key: Option<&str>, - query_pairs: Option>, - ) -> Url { - let mut url = self.client.service_url.clone(); - - // `path_segments_mut` can only error if the url is cannot-be-a-base, - // and we check that in `ClientBuilder::new`, therefore this will never panic. - let mut segments = url.path_segments_mut().unwrap(); - segments - .push("v1") - .push(match suffix { - Some("parts") => "objects:multipart:parts", - Some("complete") => "objects:multipart:complete", - _ => "objects:multipart", - }) - .push(self.scope.usecase.name()) - .push(&self.scope.scopes.as_api_path().to_string()); - if let Some(object_key) = object_key.filter(|key| !key.is_empty()) { - segments.extend(object_key.split('/')); - } - drop(segments); - if let Some(query_pairs) = query_pairs { - let mut pairs = url.query_pairs_mut(); - for (key, value) in query_pairs { - pairs.append_pair(key, &value); - } - } - - url - } - fn prepare_builder(&self, mut builder: RequestBuilder) -> crate::Result { if let Some(token) = self.get_token()? { builder = builder.header("x-os-auth", format!("Bearer {token}")); @@ -611,19 +576,6 @@ impl Session { let builder = self.client.reqwest.post(url); self.prepare_builder(builder) } - - #[cfg(feature = "multipart")] - pub(crate) fn multipart_request( - &self, - method: reqwest::Method, - action: Option<&'static str>, - object_key: Option<&str>, - query_pairs: Option>, - ) -> crate::Result { - let url = self.multipart_url(action, object_key, query_pairs); - let builder = self.client.reqwest.request(method, url); - self.prepare_builder(builder) - } } #[cfg(test)] diff --git a/clients/rust/src/error.rs b/clients/rust/src/error.rs index 1ccf46d9..7b2124b4 100644 --- a/clients/rust/src/error.rs +++ b/clients/rust/src/error.rs @@ -55,20 +55,6 @@ pub enum Error { /// The error message. message: String, }, - /// Error when part number validation fails (must be >= 1). - #[error("invalid part number: {0}")] - InvalidPartNumber(u32), - /// Error when upload ID validation fails. - #[error(transparent)] - InvalidUploadId(#[from] objectstore_types::multipart::InvalidUploadId), - /// Error returned when attempting to complete a multipart upload. - #[error("multipart complete failed ({code}): {message}")] - MultipartComplete { - /// The error code or kind. - code: String, - /// The error message. - message: String, - }, /// A resumable upload session expired, was canceled, or could otherwise not be found. #[cfg(feature = "resumable-upload-api")] #[error("resumable upload session is not available")] diff --git a/clients/rust/src/get.rs b/clients/rust/src/get.rs index 02b8b72e..579213b8 100644 --- a/clients/rust/src/get.rs +++ b/clients/rust/src/get.rs @@ -137,9 +137,6 @@ pub(crate) fn maybe_decompress( (Some(Compression::Zstd), true) => { metadata.compression = None; let mut decoder = ZstdDecoder::new(StreamReader::new(stream)); - // Multipart uploads with compression, when each part is compressed individually, - // will consist of multiple concatenated zstd frames. - // This allows the client to handle automatic decompression for these objects transparently. decoder.multiple_members(true); ReaderStream::new(decoder).boxed() } diff --git a/clients/rust/src/lib.rs b/clients/rust/src/lib.rs index 32649572..2924b257 100644 --- a/clients/rust/src/lib.rs +++ b/clients/rust/src/lib.rs @@ -11,8 +11,6 @@ mod get; mod head; mod key; mod many; -#[cfg(feature = "multipart")] -mod multipart; mod put; mod response; #[cfg(feature = "resumable-upload-api")] @@ -31,8 +29,6 @@ pub use get::*; pub use head::*; pub use key::*; pub use many::*; -#[cfg(feature = "multipart")] -pub use multipart::*; pub use put::*; #[cfg(feature = "resumable-upload-api")] pub use resumable::*; diff --git a/clients/rust/src/multipart.rs b/clients/rust/src/multipart.rs deleted file mode 100644 index 08a7ee39..00000000 --- a/clients/rust/src/multipart.rs +++ /dev/null @@ -1,422 +0,0 @@ -use std::borrow::Cow; -use std::collections::BTreeMap; - -use base64::Engine as _; -use bytes::Bytes; -use futures_util::StreamExt as _; -use objectstore_types::metadata::Metadata; -use objectstore_types::multipart::{ - CompleteErrorDetail, CompleteRequest, CompleteSuccessResponse, InitiateResponse, - ListPartsResponse, UploadPartResponse, -}; -use reqwest::Body; -use serde::Deserialize; -use tokio::io::AsyncRead; -use tokio_util::io::ReaderStream; - -use crate::response::ResponseExt as _; -use crate::{ClientStream, ObjectKey, Session}; - -pub use objectstore_types::multipart::CompletePart; -pub use objectstore_types::multipart::ETag; -pub use objectstore_types::multipart::PartInfo; -pub use objectstore_types::multipart::PartNumber; -pub use objectstore_types::multipart::UploadId; - -#[derive(Deserialize)] -#[serde(untagged)] -enum CompleteResponse { - Error { error: CompleteErrorDetail }, - Success(CompleteSuccessResponse), -} - -impl Session { - /// Creates a builder for initiating a multipart upload. - /// - /// The returned [`InitiateMultipartBuilder`] inherits the session's default compression - /// and expiration settings. - /// - /// IMPORTANT: unlike single-object uploads, the client does not automatically compress the - /// contents of [`MultipartUpload::put`]/[`MultipartUpload::put_stream`] based on the - /// configured `compression`. - /// The caller is responsible to compress the payload in accordance with the configured - /// `compression`. - /// That's because we require `content_length` on each part to be the length of the compressed - /// content, which we wouldn't be able to know beforehand if `objectstore_client` automatically - /// compressed payloads on the fly. - pub fn initiate_multipart_upload(&self) -> InitiateMultipartBuilder { - let metadata = Metadata { - expiration_policy: self.scope.usecase().expiration_policy(), - compression: self.scope.usecase().compression(), - ..Default::default() - }; - - InitiateMultipartBuilder { - session: self.clone(), - metadata, - key: None, - } - } - - /// Resumes an existing multipart upload from its key and upload ID. - /// - /// This reconstructs a [`MultipartUpload`] handle from previously obtained identifiers, and - /// doesn't make any network calls. - /// Use this to resume an upload after a process restart or to continue an upload initiated elsewhere. - pub fn resume_multipart_upload( - &self, - key: impl Into, - upload_id: impl Into, - ) -> crate::Result { - Ok(MultipartUpload { - session: self.clone(), - key: key.into(), - upload_id: UploadId::new(upload_id.into())?, - }) - } -} - -/// A builder for initiating a multipart upload. -#[derive(Debug)] -pub struct InitiateMultipartBuilder { - session: Session, - metadata: Metadata, - key: Option, -} - -impl InitiateMultipartBuilder { - /// Sets an explicit object key. - /// - /// If a key is specified, the object will be stored under that key. Otherwise, the Objectstore - /// server will automatically assign a random key, which is then returned from this request. - pub fn key(mut self, key: impl Into) -> Self { - self.key = Some(key.into()).filter(|k| !k.is_empty()); - self - } - - /// Sets the compression algorithm recorded in this object's metadata. - /// - /// IMPORTANT: unlike single-object uploads, the client does not automatically compress the - /// contents of [`MultipartUpload::put`]/[`MultipartUpload::put_stream`] based on the - /// configured `compression`. - /// The caller is responsible to compress the payload in accordance with the configured - /// `compression`. - /// - /// By default, the compression algorithm set on this Session's Usecase is used. - pub fn compression(mut self, compression: impl Into>) -> Self { - self.metadata.compression = compression.into(); - self - } - - /// Sets the expiration policy of the object to be uploaded. - /// - /// By default, the expiration policy set on this Session's Usecase is used. - pub fn expiration_policy(mut self, expiration_policy: crate::ExpirationPolicy) -> Self { - self.metadata.expiration_policy = expiration_policy; - self - } - - /// Sets the content type of the object to be uploaded. - /// - /// You can use the utility function [`crate::utils::guess_mime_type`] to attempt to guess a - /// `content_type` based on magic bytes. - pub fn content_type(mut self, content_type: impl Into>) -> Self { - self.metadata.content_type = content_type.into(); - self - } - - /// Sets the origin of the object, typically the IP address of the original source. - /// - /// This is an optional but encouraged field that tracks where the payload was - /// originally obtained from. For example, the IP address of the Sentry SDK or CLI - /// that uploaded the data. - /// - /// # Example - /// - /// ```no_run - /// # async fn example(session: objectstore_client::Session) { - /// session.initiate_multipart_upload() - /// .origin("203.0.113.42") - /// .send() - /// .await - /// .unwrap(); - /// # } - /// ``` - pub fn origin(mut self, origin: impl Into) -> Self { - self.metadata.origin = Some(origin.into()); - self - } - - /// Sets the filename of the object. - /// - /// When present, the server will include a `Content-Disposition: attachment; filename=""` - /// header in GET responses. - pub fn filename(mut self, filename: impl Into) -> Self { - self.metadata.filename = Some(filename.into()); - self - } - - /// Sets the custom metadata to the provided map. - /// - /// It will clear any previously set metadata. - pub fn set_metadata(mut self, metadata: impl Into>) -> Self { - self.metadata.custom = metadata.into(); - self - } - - /// Appends the `key`/`value` to the custom metadata of this object. - pub fn append_metadata(mut self, key: impl Into, value: impl Into) -> Self { - self.metadata.custom.insert(key.into(), value.into()); - self - } - - /// Sends the initiate request and returns a [`MultipartUpload`] handle. - pub async fn send(self) -> crate::Result { - let method = match self.key { - Some(_) => reqwest::Method::PUT, - None => reqwest::Method::POST, - }; - - let mut builder = - self.session - .multipart_request(method, None, self.key.as_deref(), None)?; - - builder = builder.headers(self.metadata.to_headers("")?); - - let response: InitiateResponse = builder - .send() - .await? - .error_for_status_and_drain() - .await? - .json() - .await?; - - Ok(MultipartUpload { - session: self.session, - key: response.key, - upload_id: response.upload_id, - }) - } -} - -/// Represents an ongoing Multipart Upload, tied to a specific [`Session`] and [`UploadId`]. -/// -/// Create a Multipart Upload handle using [`Session::initiate_multipart_upload`] or [`Session::resume_multipart_upload`]. -#[derive(Debug)] -pub struct MultipartUpload { - session: Session, - key: String, - upload_id: UploadId, -} - -impl MultipartUpload { - /// Returns the upload session identifier. - pub fn upload_id(&self) -> &UploadId { - &self.upload_id - } - - /// Returns the key of the object that this upload will create. - pub fn key(&self) -> &ObjectKey { - &self.key - } - - /// Uploads a part using a [`Bytes`]-like payload. - /// - /// IMPORTANT: unlike single-object uploads, the client does not automatically compress - /// contents based on this upload's `Metadata::compression`. - /// The caller is responsible to compress the payload in accordance with the `compression`, - /// and, optionally, to pass the `content_md5` of the compressed payload. - pub async fn put( - &self, - body: impl Into, - part_number: u32, - content_md5: Option<&[u8; 16]>, - ) -> crate::Result { - let bytes = body.into(); - let content_length = bytes.len() as u64; - self.upload_part(bytes.into(), part_number, content_length, content_md5) - .await - } - - /// Uploads a part using a streaming payload. - /// - /// IMPORTANT: unlike single-object uploads, the client does not automatically compress - /// contents based on this upload's `Metadata::compression`. - /// The caller is responsible to compress the payload in accordance with the `compression`, - /// and to pass the `content_length` and, optionally, `content_md5` of the compressed payload. - pub async fn put_stream( - &self, - stream: ClientStream, - part_number: u32, - content_length: u64, - content_md5: Option<&[u8; 16]>, - ) -> crate::Result { - self.upload_part( - Body::wrap_stream(stream), - part_number, - content_length, - content_md5, - ) - .await - } - - /// Uploads a part from an [`AsyncRead`] source. - /// - /// IMPORTANT: unlike single-object uploads, the client does not automatically compress - /// contents based on this upload's `Metadata::compression`. - /// The caller is responsible to compress the payload in accordance with the `compression`, - /// and to pass the `content_length` and, optionally, `content_md5` of the compressed payload. - pub async fn put_read( - &self, - reader: R, - part_number: u32, - content_length: u64, - content_md5: Option<&[u8; 16]>, - ) -> crate::Result - where - R: AsyncRead + Send + Sync + 'static, - { - let stream = ReaderStream::new(reader).boxed(); - self.put_stream(stream, part_number, content_length, content_md5) - .await - } - - async fn upload_part( - &self, - body: Body, - part_number: u32, - content_length: u64, - content_md5: Option<&[u8; 16]>, - ) -> crate::Result { - let part_number = - PartNumber::new(part_number).ok_or(crate::Error::InvalidPartNumber(part_number))?; - - let mut builder = self - .session - .multipart_request( - reqwest::Method::PUT, - Some("parts"), - Some(&self.key), - Some(vec![ - ("upload_id", self.upload_id.to_string()), - ("part_number", part_number.to_string()), - ]), - )? - .header(reqwest::header::CONTENT_LENGTH, content_length) - .body(body); - - if let Some(md5) = content_md5 { - let encoded = base64::engine::general_purpose::STANDARD.encode(md5); - builder = builder.header("content-md5", encoded); - } - - let response: UploadPartResponse = builder - .send() - .await? - .error_for_status_and_drain() - .await? - .json() - .await?; - Ok(CompletePart { - part_number, - etag: response.etag, - }) - } - - /// Lists all parts that have been uploaded for this multipart upload. - pub async fn list_parts(&self) -> crate::Result> { - let mut all_parts = Vec::new(); - let mut marker = None; - - loop { - let page = self.list_parts_page(None, marker).await?; - all_parts.extend(page.parts); - - if !page.is_truncated { - return Ok(all_parts); - } - marker = page.next_part_number_marker; - if marker.is_none() { - return Err(crate::Error::MalformedResponse( - "server returned is_truncated=true but no next_part_number_marker. Please report a bug.".into(), - )); - } - } - } - - async fn list_parts_page( - &self, - max_parts: Option, - part_number_marker: Option, - ) -> crate::Result { - let mut params: Vec<(&str, String)> = vec![("upload_id", self.upload_id.to_string())]; - if let Some(max) = max_parts { - params.push(("max_parts", max.to_string())); - } - if let Some(marker) = part_number_marker { - params.push(("part_number_marker", marker.to_string())); - } - - let builder = self.session.multipart_request( - reqwest::Method::GET, - Some("parts"), - Some(&self.key), - Some(params), - )?; - - let response: ListPartsResponse = builder - .send() - .await? - .error_for_status_and_drain() - .await? - .json() - .await?; - Ok(response) - } - - /// Aborts this multipart upload. - pub async fn abort(self) -> crate::Result<()> { - let builder = self.session.multipart_request( - reqwest::Method::DELETE, - None, - Some(&self.key), - Some(vec![("upload_id", self.upload_id.to_string())]), - )?; - builder - .send() - .await? - .error_for_status_and_drain() - .await? - .drain_body() - .await; - Ok(()) - } - - /// Completes the multipart upload, assembling all parts into the final object. - pub async fn complete( - self, - parts: impl IntoIterator, - ) -> crate::Result { - let mut parts: Vec<_> = parts.into_iter().collect(); - parts.sort_by_key(|p| p.part_number); - - let builder = self - .session - .multipart_request( - reqwest::Method::POST, - Some("complete"), - Some(&self.key), - Some(vec![("upload_id", self.upload_id.to_string())]), - )? - .json(&CompleteRequest { parts }); - - let response = builder.send().await?.error_for_status_and_drain().await?; - match response.json::().await? { - CompleteResponse::Success(s) => Ok(s.key), - CompleteResponse::Error { error } => Err(crate::Error::MultipartComplete { - code: error.code, - message: error.message, - }), - } - } -} diff --git a/clients/rust/tests/multipart.rs b/clients/rust/tests/multipart.rs deleted file mode 100644 index 08610656..00000000 --- a/clients/rust/tests/multipart.rs +++ /dev/null @@ -1,264 +0,0 @@ -//! End-to-end tests for the multipart upload client API. - -mod common; - -use common::{test_server, test_session}; -use futures_util::StreamExt as _; -use futures_util::stream; -use objectstore_client::{Client, CompletePart, Compression, Error, PartNumber, Usecase}; - -use crate::common::test_token_generator; - -#[tokio::test] -async fn test_full_upload_uncompressed() { - let server = test_server().await; - let client = Client::builder(server.url("/")) - .token(test_token_generator()) - .build() - .unwrap(); - let usecase = Usecase::new("usecase").with_compression(None); - - let session = client.session(usecase.for_organization(12345)).unwrap(); - - let upload = session - .initiate_multipart_upload() - .key("multipart-test-key") - .send() - .await - .unwrap(); - - assert_eq!(upload.key(), "multipart-test-key"); - assert!(!upload.upload_id().is_empty()); - - let parts_data: Vec<(&[u8], u32)> = vec![(b"hello ", 1), (b"world!", 2)]; - - let results: Vec<_> = stream::iter( - parts_data - .into_iter() - .map(|(data, part_number)| upload.put(data, part_number, None)), - ) - .buffer_unordered(2) - .collect() - .await; - - let mut parts = Vec::new(); - let mut errors = Vec::new(); - for result in results { - match result { - Ok(part) => parts.push(part), - Err(e) => errors.push(e), - } - } - assert!(errors.is_empty(), "part uploads failed: {errors:?}"); - - let key = upload.complete(parts).await.unwrap(); - - assert_eq!(key, "multipart-test-key"); - - let response = session - .get(&key) - .decompress(false) - .send() - .await - .unwrap() - .unwrap(); - assert_eq!(response.metadata.compression, None); - let payload = response.payload().await.unwrap(); - assert_eq!(payload, "hello world!"); -} - -#[tokio::test] -async fn test_full_upload_compressed() { - let server = test_server().await; - let session = test_session(&server); - - let upload = session - .initiate_multipart_upload() - .key("multipart-compressed-key") - .compression(Compression::Zstd) - .send() - .await - .unwrap(); - - let part1_data = b"hello "; - let part2_data = b"world!"; - - let parts_data: Vec<(Vec, u32)> = vec![ - (zstd::encode_all(&part1_data[..], 0).unwrap(), 1), - (zstd::encode_all(&part2_data[..], 0).unwrap(), 2), - ]; - - let results: Vec<_> = stream::iter( - parts_data - .into_iter() - .map(|(data, part_number)| upload.put(data, part_number, None)), - ) - .buffer_unordered(2) - .collect() - .await; - - let mut parts = Vec::new(); - let mut errors = Vec::new(); - for result in results { - match result { - Ok(part) => parts.push(part), - Err(e) => errors.push(e), - } - } - assert!(errors.is_empty(), "part uploads failed: {errors:?}"); - - let key = upload.complete(parts).await.unwrap(); - - let response = session - .get(&key) - .decompress(false) - .send() - .await - .unwrap() - .unwrap(); - assert_eq!(response.metadata.compression, Some(Compression::Zstd)); - - let mut expected = zstd::encode_all(&part1_data[..], 0).unwrap(); - expected.extend(zstd::encode_all(&part2_data[..], 0).unwrap()); - assert_eq!( - response.payload().await.unwrap().as_ref(), - expected.as_slice() - ); - - let response = session.get(&key).send().await.unwrap().unwrap(); - assert_eq!(response.metadata.compression, None); - assert_eq!(response.payload().await.unwrap(), "hello world!"); -} - -#[tokio::test] -async fn test_server_generated_key() { - let server = test_server().await; - let session = test_session(&server); - - let upload = session - .initiate_multipart_upload() - .compression(None) - .send() - .await - .unwrap(); - - assert!(!upload.key().is_empty()); - - let part = upload.put(b"data".as_slice(), 1, None).await.unwrap(); - - let key = upload.complete([part]).await.unwrap(); - - assert!(!key.is_empty()); - - let response = session.get(&key).send().await.unwrap().unwrap(); - assert_eq!(response.payload().await.unwrap(), "data"); -} - -#[tokio::test] -async fn test_list_parts() { - let server = test_server().await; - let session = test_session(&server); - - let upload = session - .initiate_multipart_upload() - .key("list-parts-key") - .compression(None) - .send() - .await - .unwrap(); - - upload.put(b"part-two".as_slice(), 2, None).await.unwrap(); - upload.put(b"part-one".as_slice(), 1, None).await.unwrap(); - - let parts = upload.list_parts().await.unwrap(); - assert_eq!(parts.len(), 2); - - let p1 = parts - .iter() - .find(|p| p.part_number.get() == 1) - .expect("missing part 1"); - let p2 = parts - .iter() - .find(|p| p.part_number.get() == 2) - .expect("missing part 2"); - assert_eq!(p1.size, 8); - assert_eq!(p2.size, 8); - - upload.abort().await.unwrap(); -} - -#[tokio::test] -async fn test_abort() { - let server = test_server().await; - let session = test_session(&server); - - let upload = session - .initiate_multipart_upload() - .key("abort-key") - .send() - .await - .unwrap(); - - upload.put(b"some data".as_slice(), 1, None).await.unwrap(); - upload.abort().await.unwrap(); -} - -#[tokio::test] -async fn test_metadata_preserved() { - let server = test_server().await; - let session = test_session(&server); - - let upload = session - .initiate_multipart_upload() - .key("metadata-key") - .compression(None) - .content_type("text/plain") - .origin("203.0.113.42") - .append_metadata("my-key".to_string(), "my-value".to_string()) - .send() - .await - .unwrap(); - - let part = upload.put(b"payload".as_slice(), 1, None).await.unwrap(); - - let key = upload.complete([part]).await.unwrap(); - - let response = session.get(&key).send().await.unwrap().unwrap(); - assert_eq!(response.metadata.content_type, "text/plain"); - assert_eq!(response.metadata.origin.as_deref(), Some("203.0.113.42")); - assert_eq!( - response.metadata.custom.get("my-key").map(String::as_str), - Some("my-value") - ); -} - -#[tokio::test] -async fn test_complete_with_bad_etag() { - let server = test_server().await; - let session = test_session(&server); - - let upload = session - .initiate_multipart_upload() - .key("bad-etag-key") - .compression(None) - .send() - .await - .unwrap(); - - upload.put(b"real data".as_slice(), 1, None).await.unwrap(); - - let result = upload - .complete(vec![CompletePart { - part_number: PartNumber::new(1).unwrap(), - etag: "bogus-etag".to_string(), - }]) - .await; - - match result { - Err(Error::MultipartComplete { code, message }) => { - assert!(!code.is_empty(), "error code should not be empty"); - assert!(!message.is_empty(), "error message should not be empty"); - } - other => panic!("expected MultipartComplete error, got: {other:?}"), - } -} diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index 1af031fe..dc8b9562 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -11,9 +11,8 @@ All object operations live under the `/v1/` prefix. Objects are addressed by a usecase, a set of scopes, and a key; scopes are encoded in the URL path using Matrix URI syntax (`org=123;project=456`). -Four families of routes exist: object operations, resumable uploads, multipart -uploads (being replaced by resumable uploads), and internal probes that stay -available when the server is under load. +The server exposes object operations, batch operations, resumable uploads, and +internal probes that stay available when the server is under load. See the [`endpoints`] module for the routing table and the request and response shape of every route. diff --git a/objectstore-server/src/auth/service.rs b/objectstore-server/src/auth/service.rs index 8f7b6513..098647f9 100644 --- a/objectstore-server/src/auth/service.rs +++ b/objectstore-server/src/auth/service.rs @@ -1,9 +1,5 @@ use objectstore_service::backend::common::{ExpiryUpdate, SetExpiryResponse}; use objectstore_service::id::{ObjectContext, ObjectId}; -use objectstore_service::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use objectstore_service::service::{ CreateUploadSessionResponse, DeleteResponse, GetResponse, InsertResponse, MetadataResponse, }; @@ -142,82 +138,6 @@ impl AuthAwareService { Ok(self.service.delete_object(id, access_time).await?) } - // --- Multipart upload operations --- - - /// Auth-aware wrapper around [`StorageService::initiate_multipart`]. - pub async fn initiate_multipart( - &self, - id: ObjectId, - metadata: Metadata, - ) -> ApiResult { - self.check_permission(Permission::ObjectWrite, id.context())?; - Ok(self.service.initiate_multipart(id, metadata).await?) - } - - /// Auth-aware wrapper around [`StorageService::upload_part`]. - pub async fn upload_part( - &self, - id: ObjectId, - upload_id: UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option, - body: ClientStream, - ) -> ApiResult { - self.check_permission(Permission::ObjectWrite, id.context())?; - Ok(self - .service - .upload_part( - id, - upload_id, - part_number, - content_length, - content_md5, - body, - ) - .await?) - } - - /// Auth-aware wrapper around [`StorageService::list_parts`]. - pub async fn list_parts( - &self, - id: ObjectId, - upload_id: UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> ApiResult { - self.check_permission(Permission::ObjectWrite, id.context())?; - Ok(self - .service - .list_parts(id, upload_id, max_parts, part_number_marker) - .await?) - } - - /// Auth-aware wrapper around [`StorageService::abort_multipart`]. - pub async fn abort_multipart( - &self, - id: ObjectId, - upload_id: UploadId, - ) -> ApiResult { - self.check_permission(Permission::ObjectWrite, id.context())?; - Ok(self.service.abort_multipart(id, upload_id).await?) - } - - /// Auth-aware wrapper around [`StorageService::complete_multipart`]. - pub async fn complete_multipart( - &self, - id: ObjectId, - upload_id: UploadId, - parts: Vec, - access_time: Timestamp, - ) -> ApiResult { - self.check_permission(Permission::ObjectWrite, id.context())?; - Ok(self - .service - .complete_multipart(id, upload_id, parts, access_time) - .await?) - } - // --- Resumable upload operations --- /// Auth-aware wrapper around [`StorageService::create_upload_session`]. diff --git a/objectstore-server/src/config.rs b/objectstore-server/src/config.rs index 4f914936..b5420f4f 100644 --- a/objectstore-server/src/config.rs +++ b/objectstore-server/src/config.rs @@ -72,7 +72,7 @@ use secrecy::{CloneableSecret, SecretBox, SerializableSecret, zeroize::Zeroize}; use serde::{Deserialize, Serialize}; pub use objectstore_log::{LevelFilter, LogFormat, LoggingConfig}; -pub use objectstore_service::backend::{MultipartUploadStorageConfig, StorageConfig}; +pub use objectstore_service::backend::{LongTermStorageConfig, StorageConfig}; use crate::killswitches::Killswitches; use crate::rate_limits::RateLimits; @@ -926,7 +926,7 @@ impl Config { mod tests { use std::io::Write; - use objectstore_service::backend::{HighVolumeStorageConfig, MultipartUploadStorageConfig}; + use objectstore_service::backend::{HighVolumeStorageConfig, LongTermStorageConfig}; use secrecy::ExposeSecret; use crate::killswitches::Killswitch; @@ -1125,7 +1125,7 @@ mod tests { let HighVolumeStorageConfig::BigTable(hv) = &c.high_volume; assert_eq!(hv.project_id, "my-project"); assert_eq!(hv.rpc_timeout, Duration::from_secs(2)); - let MultipartUploadStorageConfig::Gcs(lt) = &c.long_term else { + let LongTermStorageConfig::Gcs(lt) = &c.long_term else { panic!("expected gcs long_term"); }; assert_eq!(lt.bucket, "my-objectstore-bucket"); @@ -1156,7 +1156,7 @@ mod tests { assert_eq!(hv.instance_name, "my-instance"); assert_eq!(hv.table_name, "my-table"); assert_eq!(hv.rpc_timeout, Duration::from_millis(750)); - let MultipartUploadStorageConfig::FileSystem(lt) = &c.long_term else { + let LongTermStorageConfig::FileSystem(lt) = &c.long_term else { panic!("expected filesystem long_term"); }; assert_eq!(lt.path, Path::new("/data/lt")); diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 0ff0348d..82d12430 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -142,7 +142,6 @@ impl ApiError { ApiError::Service(error) => match error.kind() { ServiceErrorKind::InvalidMetadata - | ServiceErrorKind::InvalidUploadId | ServiceErrorKind::InvalidUploadSessionToken | ServiceErrorKind::ClientStream | ServiceErrorKind::ChunkExceedsUploadLength { .. } diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index a501bb46..fadd9c7a 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/objectstore-server/src/endpoints/mod.rs @@ -82,28 +82,6 @@ //! | `410` | The session expired or was canceled | Start a new session | //! | `501` | The server declined the resumable upload session creation for the requested object | Fall back to a regular upload | //! -//! # Multipart Upload Endpoints -//! -//! Multipart uploads are being replaced by [resumable -//! uploads](#resumable-upload-endpoints) and will be removed once all consumers have -//! migrated. See [`objectstore_types::multipart`] for the protocol types. -//! -//! | Method | Path | Description | -//! |-----------|--------------------------------------------------------------|--------------------------------------| -//! | `POST` | `/v1/objects:multipart/{usecase}/{scopes}/` | Initiate upload (server-generated key) | -//! | `PUT` | `/v1/objects:multipart/{usecase}/{scopes}/{*key}` | Initiate upload (user-provided key) | -//! | `PUT` | `/v1/objects:multipart:parts/{usecase}/{scopes}/{*key}` | Upload a part (`upload_id`, `part_number` query params) | -//! | `GET` | `/v1/objects:multipart:parts/{usecase}/{scopes}/{*key}` | List uploaded parts (`upload_id` query param) | -//! | `POST` | `/v1/objects:multipart:complete/{usecase}/{scopes}/{*key}` | Complete upload (`upload_id` query param) | -//! | `DELETE` | `/v1/objects:multipart/{usecase}/{scopes}/{*key}` | Abort upload (`upload_id` query param) | -//! -//! The initiate POST endpoint accepts both trailing-slash and non-trailing-slash forms. -//! -//! The complete endpoint returns `200 OK` immediately, with a streaming body that will -//! contain the error (if any) as JSON. Whitespace is sent in the streaming body to keep the -//! connection open. Clients must parse the body to determine the actual outcome, and not rely -//! on the status code. -//! //! # Internal Endpoints //! //! Internal endpoints are exempt from authentication, rate limiting, and the web concurrency @@ -128,7 +106,6 @@ mod batch; pub mod common; pub mod health; mod keda; -mod multipart; mod objects; #[cfg(all(target_os = "linux", feature = "profiling"))] mod profiling; @@ -146,8 +123,7 @@ pub fn is_internal_route(route: &str) -> bool { pub fn routes() -> Router { let routes_v1 = Router::new() .merge(objects::router()) - .merge(batch::router()) - .merge(multipart::router()); + .merge(batch::router()); let router = Router::new() .merge(health::router()) diff --git a/objectstore-server/src/endpoints/multipart.rs b/objectstore-server/src/endpoints/multipart.rs deleted file mode 100644 index d67deaa7..00000000 --- a/objectstore-server/src/endpoints/multipart.rs +++ /dev/null @@ -1,268 +0,0 @@ -use std::convert::Infallible; -use std::time::Duration; - -use crate::auth::AuthAwareService; -use crate::endpoints::common::{ApiError, ApiResult}; -use crate::extractors::Xt; -use crate::extractors::body::MeteredBody; -use crate::extractors::request_time::RequestTime; -use crate::state::ServiceState; -use axum::body::Body; -use axum::extract::{Query, State}; -use axum::http::{HeaderMap, StatusCode}; -use axum::response::{IntoResponse, Response}; -use axum::routing; -use axum::{Json, Router}; -use bytes::Bytes; -use futures::StreamExt; -use http::HeaderValue; -use http::header; -use objectstore_service::id::{ObjectContext, ObjectId}; -use objectstore_service::multipart::{CompletedPart, PartNumber, UploadId}; -use objectstore_types::metadata::Metadata; -use objectstore_types::multipart::{ - CompleteErrorDetail, CompleteErrorResponse, CompleteRequest, CompleteSuccessResponse, - InitiateResponse, ListPartsResponse, PartInfo, UploadPartResponse, -}; -use objectstore_types::time::Timestamp; -use serde::Deserialize; - -pub fn router() -> Router { - let initiate_no_key = routing::post(initiate_post); - Router::new() - .route( - "/objects:multipart/{usecase}/{scopes}", - initiate_no_key.clone(), - ) - .route("/objects:multipart/{usecase}/{scopes}/", initiate_no_key) - .route( - "/objects:multipart/{usecase}/{scopes}/{*key}", - routing::put(initiate_put).delete(abort), - ) - .route( - "/objects:multipart:parts/{usecase}/{scopes}/{*key}", - routing::get(list_parts).put(upload_part), - ) - .route( - "/objects:multipart:complete/{usecase}/{scopes}/{*key}", - routing::post(complete), - ) -} - -// --- Query parameter types --- - -#[derive(Debug, Deserialize)] -struct UploadPartQuery { - upload_id: UploadId, - part_number: PartNumber, -} - -#[derive(Debug, Deserialize)] -struct UploadIdQuery { - upload_id: UploadId, -} - -#[derive(Debug, Deserialize)] -struct ListPartsQuery { - upload_id: UploadId, - max_parts: Option, - part_number_marker: Option, -} - -// --- Handlers --- - -async fn initiate_put( - service: AuthAwareService, - state: State, - Xt(id): Xt, - headers: HeaderMap, - RequestTime(access_time): RequestTime, -) -> ApiResult { - initiate_inner(service, state, id, headers, access_time).await -} - -async fn initiate_post( - service: AuthAwareService, - state: State, - Xt(context): Xt, - headers: HeaderMap, - RequestTime(access_time): RequestTime, -) -> ApiResult { - let id = ObjectId::optional(context, None); - initiate_inner(service, state, id, headers, access_time).await -} - -async fn initiate_inner( - service: AuthAwareService, - State(state): State, - id: ObjectId, - headers: HeaderMap, - access_time: Timestamp, -) -> ApiResult { - // TODO: Update time_created in `complete`, when we have a Service API to mutate metadata. - let metadata = Metadata::from_insert_headers(&headers, "", access_time)?; - - state.config.usecases.validate(id.usecase(), &metadata)?; - - let upload_id = service.initiate_multipart(id.clone(), metadata).await?; - - let response = Json(InitiateResponse { - key: id.key().to_owned(), - upload_id, - }); - Ok((StatusCode::OK, response).into_response()) -} - -async fn upload_part( - service: AuthAwareService, - State(_state): State, - Xt(id): Xt, - Query(params): Query, - headers: HeaderMap, - MeteredBody(body): MeteredBody, -) -> ApiResult { - let content_length = headers - .get(header::CONTENT_LENGTH) - .and_then(|v| v.to_str().ok()) - .and_then(|v| v.parse::().ok()) - .ok_or_else(|| ApiError::client("content-length header is required"))?; - - let content_md5 = headers - .get("content-md5") - .and_then(|v| v.to_str().ok()) - .map(String::from); - - let etag = service - .upload_part( - id, - params.upload_id, - params.part_number, - content_length, - content_md5, - body, - ) - .await?; - - let response = Json(UploadPartResponse { etag }); - Ok((StatusCode::OK, response).into_response()) -} - -async fn list_parts( - service: AuthAwareService, - Xt(id): Xt, - Query(params): Query, -) -> ApiResult { - let response = service - .list_parts( - id, - params.upload_id, - params.max_parts, - params.part_number_marker, - ) - .await?; - - let parts = response - .parts - .into_iter() - .map(|p| PartInfo { - part_number: p.part_number, - etag: p.etag, - last_modified: p.last_modified, - size: p.size, - }) - .collect(); - - let response = Json(ListPartsResponse { - parts, - is_truncated: response.is_truncated, - next_part_number_marker: response.next_part_number_marker, - }); - Ok((StatusCode::OK, response).into_response()) -} - -async fn abort( - service: AuthAwareService, - Xt(id): Xt, - Query(params): Query, -) -> ApiResult { - service.abort_multipart(id, params.upload_id).await?; - Ok(StatusCode::NO_CONTENT.into_response()) -} - -async fn complete( - service: AuthAwareService, - Xt(id): Xt, - Query(params): Query, - RequestTime(access_time): RequestTime, - Json(body): Json, -) -> ApiResult { - let key = id.key().to_string(); - - let parts: Vec = body - .parts - .into_iter() - .map(|p| CompletedPart { - part_number: p.part_number, - etag: p.etag, - }) - .collect(); - - let upload_id = params.upload_id; - - // This operation can take a while at the service level, so we stream whitespace to the client - // until we have a response body, to keep the connection from being terminated. - let stream = async_stream::stream! { - let fut = service.complete_multipart(id, upload_id, parts, access_time); - tokio::pin!(fut); - - let mut keepalive = tokio::time::interval(Duration::from_secs(1)); - keepalive.tick().await; - loop { - tokio::select! { - res = &mut fut => { - let serialized = match res { - Ok(None) => serde_json::to_vec( - &CompleteSuccessResponse { key }, - ), - Ok(Some(err)) => serde_json::to_vec( - &CompleteErrorResponse { - error: CompleteErrorDetail { - code: err.code, - message: err.message, - }, - }, - ), - // TODO(lcian): Construct more precise error code and message, given that - // we have a structured `ApiError`. - Err(e) => serde_json::to_vec( - &CompleteErrorResponse { - error: CompleteErrorDetail { - code: "internal error".into(), - message: e.to_string(), - }, - }, - ), - }; - // Fallback to avoid `unwrap()`. This should never happen in practice. - let serialized = serialized.unwrap_or_else(|_| { - br#"{"error":{"code":"internal error","message":"unexpected error, please report a bug"}}"#.to_vec() - }); - - yield Bytes::from(serialized); - break; - } - _ = keepalive.tick() => { - yield Bytes::from_static(b" "); - } - } - } - }; - let stream = stream.map(Ok::<_, Infallible>); - - let mut headers = HeaderMap::new(); - headers.insert( - header::CONTENT_TYPE, - HeaderValue::from_static("application/json"), - ); - Ok((StatusCode::OK, headers, Body::from_stream(stream)).into_response()) -} diff --git a/objectstore-server/tests/multipart.rs b/objectstore-server/tests/multipart.rs deleted file mode 100644 index 0dbd28cd..00000000 --- a/objectstore-server/tests/multipart.rs +++ /dev/null @@ -1,651 +0,0 @@ -//! Integration tests for the multipart upload endpoints. - -use anyhow::Result; -use objectstore_server::config::{AuthZ, Config}; -use objectstore_test::server::TestServer; -use objectstore_types::multipart::{ - CompleteErrorResponse, CompleteSuccessResponse, InitiateResponse, ListPartsResponse, - PartNumber, UploadPartResponse, -}; - -fn pn(n: u32) -> PartNumber { - PartNumber::new(n).unwrap() -} - -async fn test_server() -> TestServer { - TestServer::with_config(Config { - auth: AuthZ { - enforce: false, - ..Default::default() - }, - ..Default::default() - }) - .await -} - -/// Sends a complete request and returns the trimmed response body. -/// -/// The complete endpoint uses a streaming response with keepalive whitespace, so we -/// trim the response body before returning. -async fn send_complete( - client: &reqwest::Client, - url: &str, - parts: &[(&str, &str)], -) -> Result { - let parts_json: Vec<_> = parts - .iter() - .map(|(pn, etag)| { - serde_json::json!({"part_number": pn.parse::().unwrap(), "etag": etag}) - }) - .collect(); - let body = serde_json::json!({ "parts": parts_json }); - - let response = client - .post(url) - .header("content-type", "application/json") - .body(body.to_string()) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - - let text = response.text().await?; - Ok(text.trim().to_string()) -} - -/// Sends a complete request and asserts success. Returns the key from the response. -async fn complete_and_assert( - client: &reqwest::Client, - url: &str, - parts: &[(&str, &str)], -) -> Result { - let body = send_complete(client, url, parts).await?; - let parsed: CompleteSuccessResponse = serde_json::from_str(&body) - .map_err(|e| anyhow::anyhow!("expected success response, got {body:?}: {e}"))?; - Ok(parsed.key) -} - -// --- Initiate --- - -#[tokio::test] -async fn test_initiate_put_with_key() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/my-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - - let body: InitiateResponse = response.json().await?; - assert_eq!(body.key, "my-key"); - assert!(!body.upload_id.is_empty()); - - Ok(()) -} - -#[tokio::test] -async fn test_initiate_post_generates_key() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - let response = client - .post(server.url("/v1/objects:multipart/test/org=1/")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - - let body: InitiateResponse = response.json().await?; - assert!(!body.key.is_empty()); - assert!(!body.upload_id.is_empty()); - - Ok(()) -} - -// --- Full flow: initiate → upload parts → list → complete → GET --- - -#[tokio::test] -async fn test_multipart_full_flow() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/full-flow-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - assert_eq!(initiate.key, "full-flow-key"); - - // 2. Upload part 1 - let part1_data = b"hello "; - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/full-flow-key?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", part1_data.len().to_string()) - .body(part1_data.to_vec()) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let part1: UploadPartResponse = response.json().await?; - assert!(!part1.etag.is_empty()); - - // 3. Upload part 2 - let part2_data = b"world!"; - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/full-flow-key?upload_id={upload_id}&part_number=2" - ))) - .header("content-length", part2_data.len().to_string()) - .body(part2_data.to_vec()) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let part2: UploadPartResponse = response.json().await?; - assert!(!part2.etag.is_empty()); - - // 4. List parts - let response = client - .get(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/full-flow-key?upload_id={upload_id}" - ))) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let list: ListPartsResponse = response.json().await?; - assert_eq!(list.parts.len(), 2); - assert_eq!(list.parts[0].part_number, pn(1)); - assert_eq!(list.parts[1].part_number, pn(2)); - assert_eq!(list.parts[0].size, part1_data.len() as u64); - assert_eq!(list.parts[1].size, part2_data.len() as u64); - assert!(!list.is_truncated); - - // 5. Complete - let key = complete_and_assert( - &client, - &server.url(&format!( - "/v1/objects:multipart:complete/test/org=1/full-flow-key?upload_id={upload_id}" - )), - &[("1", &part1.etag), ("2", &part2.etag)], - ) - .await?; - assert_eq!(key, "full-flow-key"); - - // 6. Verify object is accessible via regular GET - let response = client - .get(server.url("/v1/objects/test/org=1/full-flow-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let payload = response.bytes().await?; - assert_eq!(&payload[..], b"hello world!"); - - Ok(()) -} - -// --- Initiate with POST and auto-generated key, then complete --- - -#[tokio::test] -async fn test_multipart_post_initiate_full_flow() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate with POST (server-generated key) - let response = client - .post(server.url("/v1/objects:multipart/test/org=1/")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - let key = &initiate.key; - assert!(!key.is_empty()); - - // 2. Upload a single part - let data = b"auto-keyed content"; - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/{key}?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", data.len().to_string()) - .body(data.to_vec()) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let part: UploadPartResponse = response.json().await?; - - // 3. Complete - complete_and_assert( - &client, - &server.url(&format!( - "/v1/objects:multipart:complete/test/org=1/{key}?upload_id={upload_id}" - )), - &[("1", &part.etag)], - ) - .await?; - - // 4. Verify - let response = client - .get(server.url(&format!("/v1/objects/test/org=1/{key}"))) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let payload = response.bytes().await?; - assert_eq!(&payload[..], b"auto-keyed content"); - - Ok(()) -} - -// --- Abort --- - -#[tokio::test] -async fn test_multipart_abort() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/abort-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - // 2. Upload a part - let data = b"will be aborted"; - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/abort-key?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", data.len().to_string()) - .body(data.to_vec()) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - - // 3. Abort - let response = client - .delete(server.url(&format!( - "/v1/objects:multipart/test/org=1/abort-key?upload_id={upload_id}" - ))) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT); - - // 4. Object should not exist - let response = client - .get(server.url("/v1/objects/test/org=1/abort-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND); - - Ok(()) -} - -// --- List parts with pagination --- - -#[tokio::test] -async fn test_list_parts_pagination() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/paginated-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - // 2. Upload 3 parts - for i in 1..=3 { - let data = format!("part-{i}"); - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/paginated-key?upload_id={upload_id}&part_number={i}" - ))) - .header("content-length", data.len().to_string()) - .body(data) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - } - - // 3. List with max_parts=2 (should be truncated) - let response = client - .get(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/paginated-key?upload_id={upload_id}&max_parts=2" - ))) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let page1: ListPartsResponse = response.json().await?; - assert_eq!(page1.parts.len(), 2); - assert!(page1.is_truncated); - assert!(page1.next_part_number_marker.is_some()); - - // 4. List next page - let marker = page1.next_part_number_marker.unwrap(); - let response = client - .get(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/paginated-key?upload_id={upload_id}&max_parts=2&part_number_marker={marker}" - ))) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let page2: ListPartsResponse = response.json().await?; - assert_eq!(page2.parts.len(), 1); - assert!(!page2.is_truncated); - - Ok(()) -} - -// --- Part overwrite --- - -#[tokio::test] -async fn test_upload_part_overwrite() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/overwrite-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - // 2. Upload part 1 with initial data - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/overwrite-key?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", "5") - .body("first") - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let first_etag: UploadPartResponse = response.json().await?; - - // 3. Overwrite part 1 with new data - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/overwrite-key?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", "6") - .body("second") - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let second_etag: UploadPartResponse = response.json().await?; - - // The etags should differ since the data changed - assert_ne!(first_etag.etag, second_etag.etag); - - // 4. List parts — should show only the latest version - let response = client - .get(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/overwrite-key?upload_id={upload_id}" - ))) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let list: ListPartsResponse = response.json().await?; - assert_eq!(list.parts.len(), 1); - assert_eq!(list.parts[0].part_number, pn(1)); - assert_eq!(list.parts[0].etag, second_etag.etag); - assert_eq!(list.parts[0].size, 6); - - // 5. Complete with the overwritten part - complete_and_assert( - &client, - &server.url(&format!( - "/v1/objects:multipart:complete/test/org=1/overwrite-key?upload_id={upload_id}" - )), - &[("1", &second_etag.etag)], - ) - .await?; - - // 6. Verify the final object has the overwritten data - let response = client - .get(server.url("/v1/objects/test/org=1/overwrite-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let payload = response.bytes().await?; - assert_eq!(&payload[..], b"second"); - - Ok(()) -} - -// --- Missing Content-Length on upload_part --- - -#[tokio::test] -async fn test_upload_part_missing_content_length() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::builder().no_proxy().build()?; - - // 1. Initiate - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/no-cl-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - // 2. Upload part without Content-Length — reqwest normally adds it automatically for - // fixed-size bodies, so we use a streaming body to avoid that. - let stream = - futures_util::stream::once(async { Ok::<_, std::io::Error>(bytes::Bytes::from("data")) }); - let body = reqwest::Body::wrap_stream(stream); - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/no-cl-key?upload_id={upload_id}&part_number=1" - ))) - .body(body) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::BAD_REQUEST); - - Ok(()) -} - -#[tokio::test] -async fn test_upload_part_zero_part_number() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/zero-part-upload")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/zero-part-upload?upload_id={upload_id}&part_number=0" - ))) - .header("content-length", "4") - .body("data") - .send() - .await?; - - assert_eq!(response.status(), reqwest::StatusCode::BAD_REQUEST); - - Ok(()) -} - -// --- Missing upload_id query param --- - -#[tokio::test] -async fn test_missing_upload_id_on_complete() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - let complete_body = serde_json::json!({ - "parts": [{"part_number": 1, "etag": "fake"}] - }); - let response = client - .post(server.url("/v1/objects:multipart:complete/test/org=1/some-key")) - .header("content-type", "application/json") - .body(complete_body.to_string()) - .send() - .await?; - - assert_eq!(response.status(), reqwest::StatusCode::BAD_REQUEST); - - Ok(()) -} - -#[tokio::test] -async fn test_complete_zero_part_number() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/zero-part-complete")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - let response = client - .post(server.url(&format!( - "/v1/objects:multipart:complete/test/org=1/zero-part-complete?upload_id={upload_id}" - ))) - .header("content-type", "application/json") - .body(r#"{"parts":[{"part_number":0,"etag":"fake"}]}"#) - .send() - .await?; - - assert_eq!(response.status(), reqwest::StatusCode::UNPROCESSABLE_ENTITY); - Ok(()) -} - -#[tokio::test] -async fn test_missing_upload_id_on_abort() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - let response = client - .delete(server.url("/v1/objects:multipart/test/org=1/some-key")) - .send() - .await?; - - assert_eq!(response.status(), reqwest::StatusCode::BAD_REQUEST); - - Ok(()) -} - -// --- Complete with invalid etag --- - -#[tokio::test] -async fn test_complete_invalid_etag() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/bad-etag-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - - // 2. Upload a part - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/bad-etag-key?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", "4") - .body("data") - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - - // 3. Complete with a wrong etag - let body = send_complete( - &client, - &server.url(&format!( - "/v1/objects:multipart:complete/test/org=1/bad-etag-key?upload_id={upload_id}" - )), - &[("1", "wrong-etag")], - ) - .await?; - - let err: CompleteErrorResponse = serde_json::from_str(&body)?; - assert!(!err.error.code.is_empty()); - assert!(!err.error.message.is_empty()); - - // 4. Object should not exist - let response = client - .get(server.url("/v1/objects/test/org=1/bad-etag-key")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND); - - Ok(()) -} - -// --- Structured key with slashes --- - -#[tokio::test] -async fn test_multipart_structured_key() -> Result<()> { - let server = test_server().await; - let client = reqwest::Client::new(); - - // 1. Initiate with a key containing slashes - let response = client - .put(server.url("/v1/objects:multipart/test/org=1/path/to/object")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let initiate: InitiateResponse = response.json().await?; - let upload_id = &initiate.upload_id; - assert_eq!(initiate.key, "path/to/object"); - - // 2. Upload a part - let data = b"structured key data"; - let response = client - .put(server.url(&format!( - "/v1/objects:multipart:parts/test/org=1/path/to/object?upload_id={upload_id}&part_number=1" - ))) - .header("content-length", data.len().to_string()) - .body(data.to_vec()) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let part: UploadPartResponse = response.json().await?; - - // 3. Complete - complete_and_assert( - &client, - &server.url(&format!( - "/v1/objects:multipart:complete/test/org=1/path/to/object?upload_id={upload_id}" - )), - &[("1", &part.etag)], - ) - .await?; - - // 4. Verify via regular GET - let response = client - .get(server.url("/v1/objects/test/org=1/path/to/object")) - .send() - .await?; - assert_eq!(response.status(), reqwest::StatusCode::OK); - let payload = response.bytes().await?; - assert_eq!(&payload[..], b"structured key data"); - - Ok(()) -} diff --git a/objectstore-server/tests/resumable.rs b/objectstore-server/tests/resumable.rs index 3cc706a0..6b689648 100644 --- a/objectstore-server/tests/resumable.rs +++ b/objectstore-server/tests/resumable.rs @@ -151,7 +151,7 @@ async fn unknown_upload_type_is_rejected() -> Result<()> { let server = test_server().await; let response = reqwest::Client::new() - .put(server.url("/v1/objects/test/org=1/my-key?upload_type=multipart")) + .put(server.url("/v1/objects/test/org=1/my-key?upload_type=unknown")) .header(HEADER_UPLOAD_LENGTH, "1048576") .send() .await?; diff --git a/objectstore-service/Cargo.toml b/objectstore-service/Cargo.toml index d70938d9..171bd0e8 100644 --- a/objectstore-service/Cargo.toml +++ b/objectstore-service/Cargo.toml @@ -12,7 +12,6 @@ publish = false [dependencies] anyhow = { workspace = true } async-trait = { workspace = true } -base64 = { workspace = true } bigtable_rs = { workspace = true } blake3 = { workspace = true } bytes = { workspace = true } diff --git a/objectstore-service/docs/architecture.md b/objectstore-service/docs/architecture.md index 145ab78c..db913246 100644 --- a/objectstore-service/docs/architecture.md +++ b/objectstore-service/docs/architecture.md @@ -125,8 +125,8 @@ COGS pipelines). To calculate, for example, the compute costs for the ## Compute COGS Objectstore emits the `objectstore.cogs.usage` counter with an `app_feature` -label derived from the usecase once per operation. Multipart and batch -operations are also counted. This counter can be straightforwardly summed by +label derived from the usecase once per operation. Batch operations are also +counted. This counter can be straightforwardly summed by `app_feature`. The counter is incremented in the [`CountingBackend`](backend::counting::CountingBackend) @@ -194,7 +194,7 @@ three operations: Writes and updates are reported twice: `begin_*` before the storage operation and `commit_*` after it succeeds. Deletes are only committed. A failed `begin_*` fails the request before storage is touched, while `commit_*` cannot -fail. Resumable and multipart uploads call `begin_write` when they are created. +fail. Resumable uploads call `begin_write` when they are created. A `begin_*` is not always followed by a `commit_*`, so `begin_*` gives an upper bound on what is stored, e.g. for cleaning up expired data. @@ -347,14 +347,6 @@ granularity unit are rejected; the final chunk is exempt. Clients should use gra multiples and continue from the preceding response's offset, or perform an explicit offset query after an ambiguous failure. -## Multipart Uploads - -When the configured backend supports it, [`StorageService`] exposes multipart -upload operations (initiate, upload part, list parts, complete, abort). These -delegate to the [`MultipartUploadBackend`](backend::common::MultipartUploadBackend) -trait, accessed via [`Backend::as_multipart_upload_backend`](backend::common::Backend::as_multipart_upload_backend). -Multipart operations share the same concurrency limiter as regular operations. - ## Streaming Concurrency The [`streaming`](streaming) module provides [`StreamExecutor`](streaming::StreamExecutor) diff --git a/objectstore-service/src/backend/changelog.rs b/objectstore-service/src/backend/changelog.rs index dd692394..c15eed95 100644 --- a/objectstore-service/src/backend/changelog.rs +++ b/objectstore-service/src/backend/changelog.rs @@ -22,14 +22,14 @@ use std::collections::HashMap; use std::fmt; use std::sync::{Arc, Mutex}; -use std::time::{Duration, SystemTime}; +use std::time::Duration; use objectstore_types::time::Timestamp; use sentry::{Hub, SentryFutureExt}; use tokio_util::task::TaskTracker; use tokio_util::task::task_tracker::TaskTrackerToken; -use crate::backend::common::{HighVolumeBackend, MultipartUploadBackend, TieredMetadata}; +use crate::backend::common::{Backend, HighVolumeBackend, TieredMetadata}; use crate::error::Result; use crate::id::ObjectId; @@ -78,10 +78,6 @@ pub struct Change { /// /// Needs cleanup on success (the CAS committed and the old blob is unreferenced). pub old: Option, - /// Earliest time at which this entry becomes eligible for cleanup. - /// - /// [`ChangeLog::scan`] filters out the entry, unless the deadline has passed. - pub cleanup_after: Option, } /// Manager for multi-step storage changes, including backends and durable log. @@ -94,7 +90,7 @@ pub struct ChangeManager { /// The backend for small objects (≤ 1 MiB). pub(crate) high_volume: Box, /// The backend for large objects (> 1 MiB). - pub(crate) long_term: Box, + pub(crate) long_term: Box, /// Durable write-ahead log for multi-step changes. pub(crate) changelog: Box, /// Tracks outstanding background cleanup operations for graceful shutdown. @@ -105,7 +101,7 @@ impl ChangeManager { /// Creates a new `ChangeManager` with the given backends and changelog. pub fn new( high_volume: Box, - long_term: Box, + long_term: Box, changelog: Box, ) -> Arc { Arc::new(Self { @@ -144,32 +140,6 @@ impl ChangeManager { }) } - /// Records the change to the log and returns a guard in the `Assembling` state. - /// - /// Behaves like [`Self::record`], except that the guard is created in the `Assembling` state. - /// Unlike other states, this guard does nothing on drop, leaving the burden of cleaning up to - /// the [`ChangeLog`]. - #[tracing::instrument(level = "debug", fields(id = ?change.id, new = ?change.new, old = ?change.old), skip_all)] - pub async fn record_assembling(self: Arc, change: Change) -> Result { - let token = self.tracker.token(); - - let id = ChangeId::new(); - self.changelog.record(&id, &change).await?; - - let state = ChangeState { - id, - change, - phase: ChangePhase::Assembling, - manager: self.clone(), - _token: token, - }; - - Ok(ChangeGuard { - state: Some(state), - hub: Hub::current(), - }) - } - /// Scans the changelog for outstanding entries and runs cleanup for each. /// /// Spawn this into a background task at startup to recover from any orphaned objects after a @@ -263,31 +233,15 @@ impl ChangeLog for InMemoryChangeLog { } async fn scan(&self) -> Result> { - let now = SystemTime::now(); let entries = self.entries.lock().expect("lock poisoned"); let result = entries .iter() - .filter(|(_, change)| match change.cleanup_after { - None => true, - Some(deadline) => now >= deadline, - }) .map(|(id, change)| (id.clone(), change.clone())) .collect(); Ok(result) } } -#[cfg(test)] -impl InMemoryChangeLog { - /// Sets [`Change::cleanup_after`] to the past for all entries, forcing them to be returned by a subsequent [`ChangeLog::scan`]. - pub fn expire_all(&self) { - let mut entries = self.entries.lock().expect("lock poisoned"); - for change in entries.values_mut() { - change.cleanup_after = Some(SystemTime::UNIX_EPOCH); - } - } -} - /// [`ChangeLog`] implementation that discards all entries. /// /// Used as the default when no durable log is configured. Provides no @@ -318,12 +272,6 @@ pub enum ChangePhase { Recovered, /// The change is recorded in the log and LT upload has started. Recorded, - /// The LT blob originated from a multipart upload and is being assembled. - /// - /// Multipart upload completion can fail, and we want the client to be able to retry it - /// without the change cleanup process racing to delete the LT blob. - /// Therefore, cleanup of changes in this phase is deferred. - Assembling, /// LT upload has succeeded and the tombstone is being updated. Written, /// The tombstone update failed due to a conflict. @@ -370,7 +318,7 @@ impl ChangeState { ChangePhase::Written => self.read_tombstone().await, ChangePhase::Lost => self.change.old.clone(), ChangePhase::Updated => self.change.new.clone(), - ChangePhase::Assembling | ChangePhase::Completed => return, // unreachable + ChangePhase::Completed => return, // unreachable }; if current != self.change.old @@ -441,12 +389,6 @@ impl Drop for ChangeState { fn drop(&mut self) { match self.phase { ChangePhase::Completed => {} - ChangePhase::Assembling => { - objectstore_log::warn!( - change = ?self.change, - "Operation dropped in Assembling state, cleanup deferred to ChangeLog recovery" - ); - } _ => { objectstore_log::error!( change = ?self.change, @@ -489,7 +431,6 @@ impl fmt::Debug for ChangeGuard { impl Drop for ChangeGuard { fn drop(&mut self) { if let Some(state) = self.state.take() - && state.phase != ChangePhase::Assembling && state.phase != ChangePhase::Completed && let Ok(handle) = tokio::runtime::Handle::try_current() { @@ -526,7 +467,6 @@ mod tests { id: make_id("object-key"), new: Some(make_id("object-key/rev1")), old: None, - cleanup_after: None, }; log.record(&id, &change).await.unwrap(); @@ -544,7 +484,6 @@ mod tests { id: make_id("object-key"), new: None, old: Some(make_id("object-key/rev1")), - cleanup_after: None, }; log.record(&id, &change).await.unwrap(); @@ -583,7 +522,6 @@ mod tests { id: make_id("crash-test"), new: Some(make_id("crash-test/rev")), old: None, - cleanup_after: None, })) .unwrap() // Runtime drops here while `guard` is still alive outside it. @@ -597,55 +535,4 @@ mod tests { let entries = rt.block_on(log.scan()).unwrap(); assert_eq!(entries.len(), 1, "log entry must persist"); } - - #[tokio::test] - async fn scan_filters_by_cleanup_after() { - let log = InMemoryChangeLog::default(); - - let ready_id = ChangeId::new(); - log.record( - &ready_id, - &Change { - id: make_id("ready"), - new: Some(make_id("ready/rev")), - old: None, - cleanup_after: None, - }, - ) - .await - .unwrap(); - - let expired_id = ChangeId::new(); - log.record( - &expired_id, - &Change { - id: make_id("expired"), - new: Some(make_id("expired/rev")), - old: None, - cleanup_after: Some(SystemTime::now() - Duration::from_secs(1)), - }, - ) - .await - .unwrap(); - - let deferred_id = ChangeId::new(); - log.record( - &deferred_id, - &Change { - id: make_id("deferred"), - new: Some(make_id("deferred/rev")), - old: None, - cleanup_after: Some(SystemTime::now() + Duration::from_hours(24)), - }, - ) - .await - .unwrap(); - - let entries = log.scan().await.unwrap(); - assert_eq!(entries.len(), 2); - - let ids: Vec<_> = entries.iter().map(|(id, _)| id).collect(); - assert!(ids.contains(&&ready_id)); - assert!(ids.contains(&&expired_id)); - } } diff --git a/objectstore-service/src/backend/common.rs b/objectstore-service/src/backend/common.rs index 6340c567..b20a701e 100644 --- a/objectstore-service/src/backend/common.rs +++ b/objectstore-service/src/backend/common.rs @@ -13,10 +13,6 @@ use bytes::Bytes; use crate::error::{Error, ErrorKind, Result}; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session}; use crate::stream::{ClientStream, PayloadStream}; @@ -256,14 +252,6 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { /// to wait for those tasks to complete. async fn join(&self) {} - /// Borrows this backend as a [`MultipartUploadBackend`] if supported. - /// - /// The default returns an [`ErrorKind::Unsupported`]. Backends that implement - /// [`MultipartUploadBackend`] should override this to return `Ok(self)`. - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - Err(ErrorKind::Unsupported.into()) - } - /// Creates a resumable upload session for the object at `id`. /// /// Object metadata and its total length are declared upfront and cannot be mutated @@ -339,60 +327,6 @@ pub trait Backend: fmt::Debug + Send + Sync + 'static { } } -/// Trait for backends that support our S3-style multipart upload protocol. -#[async_trait::async_trait] -pub trait MultipartUploadBackend: Backend + fmt::Debug + Send + Sync + 'static { - /// Initiates a new multipart upload at `id` with the given metadata. - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result; - - /// Uploads a single part of the upload identified by `(id, upload_id)`. - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option<&str>, - body: ClientStream, - ) -> Result; - - /// Lists the parts uploaded so far for `(id, upload_id)`. - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result; - - /// Aborts the upload identified by `(id, upload_id)`. - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result; - - /// Finalizes the upload identified by `(id, upload_id)` with the given - /// ordered list of parts. - /// - /// Note that this returns `Result>`. - /// It's therefore possible to get `Ok(Some(err))`, meaning that at the server level this will - /// translate to HTTP `200 OK` with an error contained in the response body. - /// We need to do it this way to mirror backends that also behave like this (namely S3 and - /// GCS). - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result; -} - /// Trait for backends that support tombstone-conditional operations. /// /// Only backends suitable for the high-volume tier of diff --git a/objectstore-service/src/backend/counting.rs b/objectstore-service/src/backend/counting.rs index 23664d24..f7a54c08 100644 --- a/objectstore-service/src/backend/counting.rs +++ b/objectstore-service/src/backend/counting.rs @@ -24,15 +24,11 @@ use objectstore_types::time::Timestamp; use objectstore_types::usecase::Usecase; use crate::backend::common::{ - Backend, DeleteResponse, ExpiryUpdate, GetResponse, MetadataResponse, MultipartUploadBackend, - PutResponse, SetExpiryResponse, + Backend, DeleteResponse, ExpiryUpdate, GetResponse, MetadataResponse, PutResponse, + SetExpiryResponse, }; use crate::error::Result; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session}; use crate::stream::ClientStream; @@ -44,14 +40,9 @@ fn count(usecase: &Usecase) { objectstore_metrics::count!("cogs.usage" += 1, app_feature = usecase.to_string()); } -/// A [`Backend`] decorator that counts each operation performed for COGS. Also implements -/// [`MultipartUploadBackend`]. See the [module documentation](self) for how it should be used. +/// A [`Backend`] decorator that counts each operation performed for COGS. /// -/// [`CountingBackend`]'s implementation clashes with how the [`MultipartUploadBackend`] trait is -/// connected to the [`Backend`] trait. The workaround is to give `CountingBackend` (up to) two -/// `Arc`s that point to the inner backend: -/// - `inner: Arc` -/// - `inner_multipart: Option>` if `inner` supports it +/// See the [module documentation](self) for how it should be used. #[derive(Debug)] pub struct CountingBackend { inner: Arc, @@ -127,11 +118,6 @@ impl Backend for CountingBackend { self.inner.join().await; } - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - self.inner.as_multipart_upload_backend()?; - Ok(self) - } - async fn create_upload_session( &self, id: &ObjectId, @@ -168,84 +154,6 @@ impl Backend for CountingBackend { } } -#[async_trait::async_trait] -impl MultipartUploadBackend for CountingBackend { - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - count(id.usecase()); - self.inner - .as_multipart_upload_backend()? - .initiate_multipart(id, metadata) - .await - } - - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - count(id.usecase()); - self.inner - .as_multipart_upload_backend()? - .upload_part( - id, - upload_id, - part_number, - content_length, - content_md5, - body, - ) - .await - } - - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - count(id.usecase()); - self.inner - .as_multipart_upload_backend()? - .list_parts(id, upload_id, max_parts, part_number_marker) - .await - } - - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - count(id.usecase()); - self.inner - .as_multipart_upload_backend()? - .abort_multipart(id, upload_id) - .await - } - - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - count(id.usecase()); - self.inner - .as_multipart_upload_backend()? - .complete_multipart(id, upload_id, parts, access_time) - .await - } -} - #[cfg(test)] mod tests { use objectstore_types::scope::{Scope, Scopes}; @@ -358,51 +266,4 @@ mod tests { "captured: {captured:?}" ); } - - #[test] - fn counts_each_multipart_operation() { - let captured = capture(async { - let backend: Arc = Arc::new(CountingBackend::new(Box::new( - InMemoryBackend::new("in-memory"), - ))); - let multipart = backend.as_multipart_upload_backend().unwrap(); - let id = object_id("attachments"); - - let upload_id = multipart - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - multipart - .upload_part( - &id, - &upload_id, - PartNumber::new(1).unwrap(), - 2, - None, - stream::single("hi"), - ) - .await - .unwrap(); - multipart - .list_parts(&id, &upload_id, None, None) - .await - .unwrap(); - multipart - .complete_multipart(&id, &upload_id, vec![], Timestamp::now()) - .await - .unwrap(); - // The upload was completed above, so aborting it is a no-op; counting - // happens on dispatch regardless, which is what this asserts. - let _ = multipart.abort_multipart(&id, &upload_id).await; - }); - - assert_eq!( - captured - .iter() - .filter(|m| m == &"cogs.usage:+1|c|#app_feature:attachments") - .count(), - 5, - "expected one count per multipart operation, captured: {captured:?}" - ); - } } diff --git a/objectstore-service/src/backend/gcs.rs b/objectstore-service/src/backend/gcs.rs index 25b3d944..2940fb5b 100644 --- a/objectstore-service/src/backend/gcs.rs +++ b/objectstore-service/src/backend/gcs.rs @@ -5,7 +5,7 @@ use std::collections::BTreeMap; use std::future::Future; use std::num::NonZeroU64; use std::sync::Arc; -use std::time::{Duration, SystemTime}; +use std::time::Duration; use std::{fmt, io}; use futures_util::{StreamExt, TryStreamExt}; @@ -14,13 +14,13 @@ use objectstore_types::headers; use objectstore_types::metadata::{ExpirationPolicy, Metadata}; use objectstore_types::range::{ByteRange, ContentRange}; use objectstore_types::time::{Rfc3339Timestamp, Timestamp}; -use reqwest::header::{HeaderMap, HeaderName}; +use reqwest::header::HeaderMap; use reqwest::{Body, IntoUrl, Method, RequestBuilder, StatusCode, Url, header, multipart}; use serde::{Deserialize, Serialize}; use crate::backend::common::{ - self, Backend, DeleteResponse, ExpiryUpdate, GetResponse, MetadataResponse, - MultipartUploadBackend, PutResponse, SetExpiryResponse, + self, Backend, DeleteResponse, ExpiryUpdate, GetResponse, MetadataResponse, PutResponse, + SetExpiryResponse, }; use crate::backend::extensions::{ReqwestResultExt, ResponseExt, SendTraced}; use crate::change_stream::{ @@ -29,10 +29,6 @@ use crate::change_stream::{ use crate::error::{Error, ErrorKind, Result, ResultExt as _}; use crate::gcp_auth::PrefetchingTokenProvider; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session, UploadProgress}; use crate::stream::ClientStream; @@ -247,8 +243,7 @@ impl GcsObject { ); } - // Free-form strings are stored escaped, even though this JSON representation could carry - // them verbatim. See `insert_gcs_meta_header` for why, and why both writers must agree. + // Preserve the stored escaping convention for free-form metadata strings. if let Some(origin) = &metadata.origin { gcs_object.metadata.insert( GcsMetaKey::Origin, @@ -410,85 +405,11 @@ impl Serialize for GcsMetaKey { } } -/// Builds HTTP headers that encode `metadata` for a GCS XML API request. -fn metadata_to_gcs_headers(metadata: &Metadata) -> Result { - let mut headers = header::HeaderMap::new(); - - if let Some(custom_time) = metadata.time_expires { - let formatted = custom_time.as_rfc3339(); - headers.insert( - HeaderName::from_static("x-goog-custom-time"), - formatted - .to_string() - .parse() - .context(ErrorKind::Internal, "encoding GCS custom-time header")?, - ); - } - - if let Some(compression) = metadata.compression { - headers.insert( - header::CONTENT_ENCODING, - compression - .to_string() - .parse() - .context(ErrorKind::Internal, "encoding GCS content-encoding header")?, - ); - } - - if metadata.expiration_policy != ExpirationPolicy::default() { - insert_gcs_meta_header( - &mut headers, - &GcsMetaKey::Expiration, - &metadata.expiration_policy.to_string(), - )?; - } - - if let Some(origin) = &metadata.origin { - insert_gcs_meta_header(&mut headers, &GcsMetaKey::Origin, origin)?; - } - - if let Some(filename) = &metadata.filename { - insert_gcs_meta_header(&mut headers, &GcsMetaKey::Filename, filename)?; - } - - for (key, value) in &metadata.custom { - insert_gcs_meta_header(&mut headers, &GcsMetaKey::Custom(key.clone()), value)?; - } - - Ok(headers) -} - /// Decodes a stored GCS metadata value into its logical string. fn decode_gcs_meta_value(value: &str, context: &'static str) -> Result { headers::decode_header_str(value).context(ErrorKind::CorruptData, context) } -/// Inserts a single `x-goog-meta-*` header, escaping the value for transport. -/// -/// Google: "you should generally avoid non-ascii characters, because they are not permitted in -/// HTTP headers, which the XML API uses" ([docs]). Real GCS does preserve raw UTF-8 here, but that -/// is undocumented, and it still drops leading whitespace and turns invalid UTF-8 into `U+FFFD`. -/// -/// [`GcsObject::from_metadata`] escapes the same values on the JSON path: reads always come back -/// through the JSON API and cannot tell which writer produced an object, so both must agree. -/// -/// [docs]: https://docs.cloud.google.com/storage/docs/metadata -fn insert_gcs_meta_header( - headers: &mut header::HeaderMap, - key: &GcsMetaKey, - value: &str, -) -> Result<()> { - let header_name = format!("x-goog-meta-{key}"); - headers.insert( - HeaderName::try_from(&header_name).context( - ErrorKind::Internal, - format!("encoding GCS metadata header {header_name}"), - )?, - headers::encode_header_value(value), - ); - Ok(()) -} - /// Special status code returned by GCS when a Resumable Upload is canceled successfully or when /// making other requests to a session that was recently canceled. const CLIENT_CLOSED_REQUEST_STATUS: u16 = 499; @@ -592,29 +513,6 @@ impl GcsBackend { Ok(url) } - /// Formats a GCS XML API URL for the given object. - /// - /// Unlike [`object_url`](Self::object_url) (JSON API at - /// `/storage/v1/b/{bucket}/o/{name}`), this produces - /// `/{bucket}/{path_segments}` for the S3-compatible XML API used by - /// multipart uploads. - fn xml_object_url(&self, id: &ObjectId) -> Result { - let mut url = self.endpoint.clone(); - { - let mut segments = url.path_segments_mut().map_err(|()| { - Error::new( - ErrorKind::Internal, - format!("building GCS object URL from {}", self.endpoint), - ) - })?; - segments.push(&self.bucket); - for part in id.as_storage_path().to_string().split('/') { - segments.push(part); - } - } - Ok(url) - } - /// Creates a request builder with the appropriate authentication. async fn request(&self, method: Method, url: impl IntoUrl) -> Result { let mut builder = self.client.request(method, url); @@ -889,10 +787,6 @@ impl Backend for GcsBackend { 256 * 1024 } - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - Ok(self) - } - #[tracing::instrument(level = "debug", fields(?id), skip_all)] async fn put_object( &self, @@ -1388,380 +1282,12 @@ impl Backend for GcsBackend { } } -#[derive(Debug, Deserialize)] -#[serde(rename_all = "PascalCase")] -struct XmlInitiateMultipartUploadResponse { - upload_id: String, -} - -impl TryFrom for InitiateMultipartResponse { - type Error = Error; - - fn try_from(r: XmlInitiateMultipartUploadResponse) -> Result { - Ok(UploadId::new(r.upload_id)?) - } -} - -#[derive(Debug, Deserialize)] -#[serde(rename_all = "PascalCase")] -struct XmlListPartsResponse { - #[serde(default)] - is_truncated: bool, - next_part_number_marker: Option, - #[serde(default, rename = "Part")] - parts: Vec, -} - -impl From for ListPartsResponse { - fn from(xml: XmlListPartsResponse) -> Self { - Self { - parts: xml.parts.into_iter().map(Into::into).collect(), - is_truncated: xml.is_truncated, - next_part_number_marker: xml.next_part_number_marker, - } - } -} - -#[derive(Debug, Deserialize)] -#[serde(rename_all = "PascalCase")] -struct XmlPart { - part_number: PartNumber, - #[serde(rename = "ETag")] - e_tag: String, - #[serde(with = "humantime_serde")] - last_modified: SystemTime, - size: u64, -} - -impl From for crate::multipart::Part { - fn from(p: XmlPart) -> Self { - Self { - part_number: p.part_number, - etag: p.e_tag, - last_modified: p.last_modified, - size: p.size, - } - } -} - -#[derive(Debug, Serialize)] -#[serde(rename = "CompleteMultipartUpload")] -struct XmlCompleteMultipartUpload { - #[serde(rename = "Part")] - parts: Vec, -} - -impl From> for XmlCompleteMultipartUpload { - fn from(parts: Vec) -> Self { - Self { - parts: parts.into_iter().map(Into::into).collect(), - } - } -} - -#[derive(Debug, Serialize)] -#[serde(rename_all = "PascalCase")] -struct XmlCompletePart { - part_number: PartNumber, - #[serde(rename = "ETag")] - e_tag: String, -} - -impl From for XmlCompletePart { - fn from(p: CompletedPart) -> Self { - Self { - part_number: p.part_number, - e_tag: p.etag, - } - } -} - -#[derive(Debug, Deserialize)] -#[serde(rename = "Error", rename_all = "PascalCase")] -struct XmlError { - code: String, - message: String, -} - -impl From for crate::multipart::CompleteMultipartError { - fn from(e: XmlError) -> Self { - Self { - code: e.code, - message: e.message, - } - } -} - -/// XXX: Any change that affects this implementation should be manually tested against real GCS. -/// That's because the fork of [storage-testbench](https://github.com/googleapis/storage-testbench) -/// that we test against has an incomplete implementation of the XML multipart API that likely doesn't match GCS's behavior in many cases. -#[async_trait::async_trait] -impl MultipartUploadBackend for GcsBackend { - #[tracing::instrument(level = "debug", fields(?id), skip_all)] - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - objectstore_log::debug!("Initiating multipart upload on GCS backend"); - self.change_stream - .begin_write(id, metadata.time_expires) - .await?; - let mut url = self.xml_object_url(id)?; - url.set_query(Some("uploads")); - - let mut headers = metadata_to_gcs_headers(metadata)?; - headers.insert( - header::CONTENT_TYPE, - metadata - .content_type - .parse() - .context(ErrorKind::InvalidMetadata, "encoding GCS content type")?, - ); - headers.insert( - header::CONTENT_LENGTH, - header::HeaderValue::from_static("0"), - ); - - self.with_retry("initiate_multipart", || { - let url = url.clone(); - let headers = headers.clone(); - async move { - let resp = self - .request(Method::POST, url) - .await? - .headers(headers) - .send_traced() - .await - .check_error("initiating a GCS multipart upload") - .await?; - - let body = resp - .bytes() - .await - .reqwest_context("reading GCS initiate-multipart response")?; - - let xml: XmlInitiateMultipartUploadResponse = - quick_xml::de::from_reader(body.as_ref()).context( - ErrorKind::CorruptData, - "decoding GCS initiate-multipart response", - )?; - - xml.try_into() - } - }) - .await - } - - #[tracing::instrument(level = "debug", skip(self, content_md5, body))] - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - objectstore_log::debug!("Uploading part to GCS backend"); - let mut url = self.xml_object_url(id)?; - url.query_pairs_mut() - .append_pair("partNumber", &part_number.to_string()) - .append_pair("uploadId", upload_id); - - let mut builder = self - .request(Method::PUT, url) - .await? - .header(header::CONTENT_LENGTH, content_length) - .body(Body::wrap_stream(body)); - - if let Some(md5) = content_md5 { - builder = builder.header("content-md5", md5); - } - - let resp = builder - .send_traced() - .await - .check_error("uploading a GCS multipart part") - .await?; - - let etag = resp - .headers() - .get(header::ETAG) - .and_then(|v| v.to_str().ok()) - .map(|s| s.to_owned()) - .ok_or_else(|| { - Error::new( - ErrorKind::BackendFailure, - "GCS upload-part response missing ETag", - ) - })?; - - resp.drain_body().await; - - Ok(etag) - } - - #[tracing::instrument(level = "debug", skip(self))] - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - objectstore_log::debug!("Listing parts on GCS backend"); - let mut url = self.xml_object_url(id)?; - { - let mut pairs = url.query_pairs_mut(); - pairs.append_pair("uploadId", upload_id); - if let Some(max) = max_parts { - pairs.append_pair("max-parts", &max.to_string()); - } - if let Some(marker) = part_number_marker { - pairs.append_pair("part-number-marker", &marker.to_string()); - } - } - - self.with_retry("list_parts", || { - let url = url.clone(); - async move { - let resp = self - .request(Method::GET, url) - .await? - .send_traced() - .await - .check_error("listing GCS multipart parts") - .await?; - - let body = resp - .bytes() - .await - .reqwest_context("reading GCS list-parts response")?; - - let xml: XmlListPartsResponse = quick_xml::de::from_reader(body.as_ref()) - .context(ErrorKind::CorruptData, "decoding GCS list-parts response")?; - - Ok(xml.into()) - } - }) - .await - } - - #[tracing::instrument(level = "debug", skip(self))] - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - objectstore_log::debug!("Aborting multipart upload on GCS backend"); - let mut url = self.xml_object_url(id)?; - url.query_pairs_mut().append_pair("uploadId", upload_id); - - self.with_retry("abort_multipart", || { - let url = url.clone(); - async move { - let resp = self - .request(Method::DELETE, url) - .await? - .send_traced() - .await - .reqwest_context("aborting a GCS multipart upload")?; - - // XXX: real S3 would return 404 here if the upload has been recently completed and we - // would have to handle it. It turns out GCS returns 204 instead, so we don't need to - // handle that case. - - resp.check_error("aborting a GCS multipart upload") - .await? - .drain_body() - .await; - - Ok(()) - } - }) - .await - } - - #[tracing::instrument(level = "debug", skip(self, parts))] - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - objectstore_log::debug!("Completing multipart upload on GCS backend"); - let mut url = self.xml_object_url(id)?; - url.query_pairs_mut().append_pair("uploadId", upload_id); - - let body = XmlCompleteMultipartUpload::from(parts); - let xml = quick_xml::se::to_string(&body).context( - ErrorKind::Internal, - "encoding GCS complete-multipart request", - )?; - - self.with_retry("complete_multipart", || { - let url = url.clone(); - let xml = xml.clone(); - async move { - let resp = self - .request(Method::POST, url) - .await? - .header(header::CONTENT_TYPE, "application/xml") - .body(xml) - .send_traced() - .await - .check_error("completing a GCS multipart upload") - .await?; - - // XXX: real S3 would return 404 here if the upload has been recently completed and we - // would have to handle it. It turns out GCS returns 200 instead, so we don't need to - // handle that case. - - let body = resp - .bytes() - .await - .reqwest_context("reading GCS complete-multipart response")?; - - let error = quick_xml::de::from_reader::<_, XmlError>(body.as_ref()) - .ok() - .map(Into::into); - - if error.is_none() { - // The XML API does not return the stored object, so read it back. - let object = self - .get_gcs_metadata(&self.object_url(id)?, access_time) - .await - .ok() - .flatten(); - let stored_size = object - .as_ref() - .and_then(|object| object.size.as_deref()?.parse().ok()); - self.report_object_write( - id, - stored_size, - object.as_ref().map_or(0, GcsObject::metadata_size), - object.and_then(|object| { - object.custom_time.map(Rfc3339Timestamp::into_inner) - }), - ) - .await; - } - - Ok(error) - } - }) - .await - } -} - #[cfg(test)] mod tests { use std::collections::BTreeMap; use std::io::{Read, Write}; use std::net::{TcpListener, TcpStream}; - use std::num::{NonZeroU32, NonZeroU64}; + use std::num::NonZeroU64; use std::sync::mpsc; use std::thread; use std::time::Duration; @@ -1775,7 +1301,6 @@ mod tests { use super::*; use crate::backend::common::ExpiryTarget; use crate::id::ObjectContext; - use crate::multipart::CompletedPart; use crate::stream; #[cfg(feature = "storage-cogs")] use crate::stream::ClientError; @@ -2408,9 +1933,6 @@ mod tests { } } - /// Both GCS write paths must agree on how a logical string is stored, because every read goes - /// through the JSON API: `put_object` writes metadata as JSON, `initiate_multipart` writes it - /// as `x-goog-meta-*` request headers. #[tokio::test] async fn test_unicode_metadata_roundtrip_json_upload() -> Result<()> { let backend = create_test_backend().await?; @@ -2433,21 +1955,6 @@ mod tests { Ok(()) } - #[tokio::test] - async fn test_unicode_metadata_roundtrip_multipart_upload() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = unicode_metadata(); - - multipart_put(&backend, &id, &metadata, "hello, world").await?; - - let meta = backend.get_metadata(&id, Timestamp::now()).await?.unwrap(); - assert_eq!(meta.filename, metadata.filename); - assert_eq!(meta.custom, metadata.custom); - - Ok(()) - } - #[test] fn from_metadata_uses_provided_time_expires() { let created = Timestamp::now(); @@ -2835,427 +2342,6 @@ mod tests { Ok(()) } - #[tokio::test] - async fn test_multipart_single_part() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata { - content_type: "text/plain".into(), - expiration_policy: ExpirationPolicy::TimeToLive(Duration::from_mins(33)), - origin: Some("203.0.113.42".into()), - custom: BTreeMap::from_iter([("hello".into(), "world".into())]), - ..Default::default() - }; - - let upload_id = backend.initiate_multipart(&id, &metadata).await?; - - let data = b"hello, multipart world!"; - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - data.len() as u64, - None, - stream::single(data.to_vec()), - ) - .await?; - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await?; - assert!(result.is_none(), "expected no error on complete"); - - let (meta, _, stream) = backend - .get_object(&id, Timestamp::now(), None) - .await? - .unwrap(); - let payload = stream::read_to_vec(stream).await?; - assert_eq!(payload, data); - assert_eq!(meta.content_type, "text/plain".to_string()); - assert_eq!( - meta.expiration_policy, - ExpirationPolicy::TimeToLive(Duration::from_mins(33)) - ); - assert_eq!(meta.origin, Some("203.0.113.42".into())); - assert_eq!( - meta.custom, - BTreeMap::from_iter([("hello".into(), "world".into())]) - ); - - Ok(()) - } - - #[tokio::test] - async fn test_multipart_multiple_parts() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await?; - - // Non-final parts must be >= 5 MiB. - const MIN_PART: usize = 5 * 1024 * 1024; - let part1 = vec![b'a'; MIN_PART]; - let part2 = vec![b'b'; MIN_PART]; - let part3 = b"cccc".to_vec(); - - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - part1.len() as u64, - None, - stream::single(part1.clone()), - ) - .await?; - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - part2.len() as u64, - None, - stream::single(part2.clone()), - ) - .await?; - let etag3 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(3).unwrap(), - part3.len() as u64, - None, - stream::single(part3.clone()), - ) - .await?; - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![ - CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag1, - }, - CompletedPart { - part_number: NonZeroU32::new(2).unwrap(), - etag: etag2, - }, - CompletedPart { - part_number: NonZeroU32::new(3).unwrap(), - etag: etag3, - }, - ], - Timestamp::now(), - ) - .await?; - assert!(result.is_none(), "expected no error on complete"); - - // Object exists after complete - let (_meta, _, stream) = backend - .get_object(&id, Timestamp::now(), None) - .await? - .unwrap(); - let payload = stream::read_to_vec(stream).await?; - let mut expected = Vec::new(); - expected.extend_from_slice(&part1); - expected.extend_from_slice(&part2); - expected.extend_from_slice(&part3); - assert_eq!(payload, expected); - - Ok(()) - } - - #[tokio::test] - async fn test_multipart_out_of_order_upload() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await?; - - // Non-final parts must be >= 5 MiB. - const MIN_PART: usize = 5 * 1024 * 1024; - let part1 = vec![b'a'; MIN_PART]; - let part2 = vec![b'b'; MIN_PART]; - let part3 = b"cccc".to_vec(); - - // Upload parts out of order: 2, 3, 1. - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - part2.len() as u64, - None, - stream::single(part2.clone()), - ) - .await?; - let etag3 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(3).unwrap(), - part3.len() as u64, - None, - stream::single(part3.clone()), - ) - .await?; - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - part1.len() as u64, - None, - stream::single(part1.clone()), - ) - .await?; - - // Complete with parts listed in order. - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![ - CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag1, - }, - CompletedPart { - part_number: NonZeroU32::new(2).unwrap(), - etag: etag2, - }, - CompletedPart { - part_number: NonZeroU32::new(3).unwrap(), - etag: etag3, - }, - ], - Timestamp::now(), - ) - .await?; - assert!(result.is_none(), "expected no error on complete"); - - // Verify reassembly order matches part numbers, not upload order. - let (_meta, _, stream) = backend - .get_object(&id, Timestamp::now(), None) - .await? - .unwrap(); - let payload = stream::read_to_vec(stream).await?; - let mut expected = Vec::new(); - expected.extend_from_slice(&part1); - expected.extend_from_slice(&part2); - expected.extend_from_slice(&part3); - assert_eq!(payload, expected); - - Ok(()) - } - - #[tokio::test] - async fn test_multipart_list_parts() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await?; - - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 3, - None, - stream::single(b"aaa".to_vec()), - ) - .await?; - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - 3, - None, - stream::single(b"bbb".to_vec()), - ) - .await?; - - // List all parts. - let list = backend.list_parts(&id, &upload_id, None, None).await?; - assert_eq!(list.parts.len(), 2); - assert_eq!(list.parts[0].part_number.get(), 1); - assert_eq!(list.parts[0].etag, etag1); - assert_eq!(list.parts[0].size, 3); - assert_eq!(list.parts[1].part_number.get(), 2); - assert_eq!(list.parts[1].etag, etag2); - assert_eq!(list.parts[1].size, 3); - - // List with max_parts=1 to test pagination. - let page1 = backend.list_parts(&id, &upload_id, Some(1), None).await?; - assert_eq!(page1.parts.len(), 1); - assert_eq!(page1.parts[0].part_number.get(), 1); - assert!(page1.is_truncated); - assert!(page1.next_part_number_marker.is_some()); - - let page2 = backend - .list_parts(&id, &upload_id, Some(1), page1.next_part_number_marker) - .await?; - assert_eq!(page2.parts.len(), 1); - assert_eq!(page2.parts[0].part_number.get(), 2); - - // Clean up. - backend.abort_multipart(&id, &upload_id).await?; - - Ok(()) - } - - #[tokio::test] - async fn test_multipart_abort() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await?; - - backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await?; - - backend.abort_multipart(&id, &upload_id).await?; - - // Object should not exist after abort. - let result = backend.get_object(&id, Timestamp::now(), None).await?; - assert!(result.is_none(), "object should not exist after abort"); - - // A second abort should still succeed (idempotent 404 handling). - backend.abort_multipart(&id, &upload_id).await?; - - Ok(()) - } - - async fn multipart_put( - backend: &GcsBackend, - id: &ObjectId, - metadata: &Metadata, - payload: impl Into, - ) -> Result<()> { - let payload: bytes::Bytes = payload.into(); - let upload_id = backend.initiate_multipart(id, metadata).await?; - let etag = backend - .upload_part( - id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload), - ) - .await?; - let error = backend - .complete_multipart( - id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await?; - assert!( - error.is_none(), - "complete_multipart returned error: {error:?}" - ); - Ok(()) - } - - #[tokio::test] - async fn test_multipart_ttl_immediate() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata { - expiration_policy: ExpirationPolicy::TimeToLive(Duration::from_secs(0)), - time_expires: Some(Timestamp::now() - Duration::from_secs(1)), - ..Default::default() - }; - - multipart_put(&backend, &id, &metadata, "hello, world").await?; - - let result = backend.get_object(&id, Timestamp::now(), None).await?; - assert!(result.is_none()); - - Ok(()) - } - - #[tokio::test] - async fn test_multipart_tti_immediate() -> Result<()> { - let backend = create_test_backend().await?; - let id = make_id(); - let metadata = Metadata { - expiration_policy: ExpirationPolicy::TimeToIdle(Duration::from_secs(0)), - time_expires: Some(Timestamp::now() - Duration::from_secs(1)), - ..Default::default() - }; - - multipart_put(&backend, &id, &metadata, "hello, world").await?; - - let result = backend.get_object(&id, Timestamp::now(), None).await?; - assert!(result.is_none()); - - Ok(()) - } - - #[tokio::test] - async fn test_multipart_compressed_payload_roundtrip() -> Result<()> { - use objectstore_types::metadata::Compression; - - let backend = create_test_backend().await?; - - let plaintext = b"hello, world (but compressed with zstd)"; - let compressed = zstd::encode_all(&plaintext[..], 3)?; - - let id = make_id(); - let metadata = Metadata { - content_type: "text/plain".into(), - compression: Some(Compression::Zstd), - ..Default::default() - }; - - multipart_put(&backend, &id, &metadata, compressed.clone()).await?; - - let (meta, _, stream) = backend - .get_object(&id, Timestamp::now(), None) - .await? - .unwrap(); - let payload = stream::read_to_vec(stream).await?; - - assert_eq!(meta.compression, Some(Compression::Zstd)); - assert_eq!( - payload, compressed, - "Payload should be returned still compressed, not auto-decompressed" - ); - - Ok(()) - } - #[cfg(feature = "storage-cogs")] #[tokio::test] async fn change_stream_reports_the_size_gcs_stored() -> Result<()> { @@ -3291,46 +2377,6 @@ mod tests { Ok(()) } - #[cfg(feature = "storage-cogs")] - #[tokio::test] - async fn multipart_completion_reports_to_change_stream() -> Result<()> { - let (backend, producer) = create_test_backend_with_change_stream().await?; - let id = make_id(); - let payload = b"multipart payload".to_vec(); - let metadata = Metadata { - time_expires: Some(Timestamp::now() + Duration::from_secs(3600)), - ..Default::default() - }; - let upload_id = backend.initiate_multipart(&id, &metadata).await?; - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload.clone()), - ) - .await?; - let part = CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }; - backend - .complete_multipart(&id, &upload_id, vec![part], Timestamp::now()) - .await?; - - let records = producer.records(); - assert_eq!(records.len(), 1); - assert_eq!(records[0].op_type, OpType::Write); - assert_eq!( - records[0].size, - Some(payload.len() as u64 + GcsObject::from_metadata(&metadata).metadata_size()) - ); - assert!(records[0].expiration_time.is_some()); - Ok(()) - } - #[cfg(feature = "storage-cogs")] #[tokio::test] async fn resumable_completion_reports_to_change_stream() -> Result<()> { diff --git a/objectstore-service/src/backend/in_memory.rs b/objectstore-service/src/backend/in_memory.rs index 54b65019..68141615 100644 --- a/objectstore-service/src/backend/in_memory.rs +++ b/objectstore-service/src/backend/in_memory.rs @@ -5,10 +5,9 @@ //! backend is [`Clone`] so tests can hold a handle for direct inspection while //! the service owns a boxed copy. -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use std::num::NonZeroU64; use std::sync::{Arc, Mutex}; -use std::time::SystemTime; use objectstore_types::range::ByteRange; use objectstore_types::time::Timestamp; @@ -18,17 +17,12 @@ use futures_util::TryStreamExt; use objectstore_types::metadata::Metadata; use crate::backend::common::{ - self, DeleteResponse, ExpiryUpdate, GetResponse, HighVolumeBackend, MultipartUploadBackend, - PutResponse, SetExpiryResponse, TieredGet, TieredMetadata, TieredUpdate, TieredWrite, - Tombstone, + self, DeleteResponse, ExpiryUpdate, GetResponse, HighVolumeBackend, PutResponse, + SetExpiryResponse, TieredGet, TieredMetadata, TieredUpdate, TieredWrite, Tombstone, }; use crate::change_stream::{ChangeStream, NoopStream, flush_change_stream}; -use crate::error::{Error, ErrorKind, Result}; +use crate::error::{ErrorKind, Result}; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, Part, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session, UploadProgress}; use crate::stream::ClientStream; @@ -63,19 +57,6 @@ impl StoreEntry { type Store = HashMap; -#[derive(Clone, Debug)] -struct MultipartUpload { - metadata: Metadata, - parts: BTreeMap, -} - -#[derive(Clone, Debug)] -struct UploadedPart { - etag: String, - data: Bytes, - uploaded_at: SystemTime, -} - #[derive(Debug)] struct ResumableUpload { metadata: Metadata, @@ -86,8 +67,6 @@ struct ResumableUpload { type ResumableSession = Arc>>; type ResumableStore = HashMap<(ObjectId, BackendToken), ResumableSession>; -type MultipartStore = HashMap<(ObjectId, UploadId), MultipartUpload>; - /// In-memory [`Backend`](super::common::Backend) backed by a `HashMap`. /// /// Removes the need for filesystem tempdir management in unit tests. The @@ -97,7 +76,6 @@ type MultipartStore = HashMap<(ObjectId, UploadId), MultipartUpload>; pub struct InMemoryBackend { name: &'static str, store: Arc>, - multipart_store: Arc>, resumable_store: Arc>, upload_markers: Arc>>, change_stream: Arc, @@ -109,7 +87,6 @@ impl InMemoryBackend { Self { name, store: Arc::new(Mutex::new(HashMap::new())), - multipart_store: Arc::new(Mutex::new(HashMap::new())), resumable_store: Arc::new(Mutex::new(HashMap::new())), upload_markers: Arc::new(Mutex::new(HashMap::new())), change_stream: Arc::new(NoopStream), @@ -204,10 +181,6 @@ impl super::common::Backend for InMemoryBackend { self.name } - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - Ok(self) - } - async fn put_object( &self, id: &ObjectId, @@ -622,197 +595,6 @@ impl HighVolumeBackend for InMemoryBackend { } } -#[async_trait::async_trait] -impl MultipartUploadBackend for InMemoryBackend { - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - self.change_stream - .begin_write(id, metadata.time_expires) - .await?; - let upload_id = UploadId::new(uuid::Uuid::now_v7().to_string())?; - let upload = MultipartUpload { - metadata: metadata.clone(), - parts: BTreeMap::new(), - }; - self.multipart_store - .lock() - .unwrap() - .insert((id.clone(), upload_id.clone()), upload); - Ok(upload_id) - } - - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - _content_length: u64, - _content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - let data: BytesMut = body.try_collect().await?; - let data = data.freeze(); - let etag = format!("\"etag-{part_number}-{}\"", data.len()); - - let mut store = self.multipart_store.lock().unwrap(); - let upload = store - .get_mut(&(id.clone(), upload_id.clone())) - .ok_or_else(|| { - Error::new( - ErrorKind::BackendFailure, - "in-memory multipart upload not found", - ) - })?; - - upload.parts.insert( - part_number, - UploadedPart { - etag: etag.clone(), - data, - uploaded_at: SystemTime::now(), - }, - ); - - Ok(etag) - } - - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - let store = self.multipart_store.lock().unwrap(); - let upload = store.get(&(id.clone(), upload_id.clone())).ok_or_else(|| { - Error::new( - ErrorKind::BackendFailure, - "in-memory multipart upload not found", - ) - })?; - - let iter = upload - .parts - .iter() - .filter(|(pn, _)| part_number_marker.is_none_or(|marker| **pn > marker)); - - let max = max_parts.unwrap_or(u32::MAX) as usize; - let all: Vec<_> = iter.collect(); - let is_truncated = all.len() > max; - let page: Vec<_> = all.into_iter().take(max).collect(); - - let next_part_number_marker = if is_truncated { - page.last().map(|(pn, _)| **pn) - } else { - None - }; - - let parts = page - .into_iter() - .map(|(pn, part)| Part { - part_number: *pn, - etag: part.etag.clone(), - last_modified: part.uploaded_at, - size: part.data.len() as u64, - }) - .collect(); - - Ok(ListPartsResponse { - parts, - is_truncated, - next_part_number_marker, - }) - } - - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - self.multipart_store - .lock() - .unwrap() - .remove(&(id.clone(), upload_id.clone())); - Ok(()) - } - - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - _access_time: Timestamp, - ) -> Result { - let key = (id.clone(), upload_id.clone()); - - // TODO: validate that parts are in ascending part_number order and reject with - // InvalidPartOrder if not (matches S3/GCS behavior). Needs a proper client error variant. - - // Validate and assemble while holding the multipart lock, but don't - // remove the upload yet — a failed validation must leave it intact so - // the client can retry. - let (metadata, payload) = { - let store = self.multipart_store.lock().unwrap(); - let upload = store.get(&key).ok_or_else(|| { - Error::new( - ErrorKind::BackendFailure, - "in-memory multipart upload not found", - ) - })?; - - for completed in &parts { - match upload.parts.get(&completed.part_number) { - None => { - return Ok(Some(crate::multipart::CompleteMultipartError { - code: "InvalidPart".into(), - message: format!( - "part number {} was not uploaded", - completed.part_number - ), - })); - } - Some(stored) if stored.etag != completed.etag => { - return Ok(Some(crate::multipart::CompleteMultipartError { - code: "InvalidPart".into(), - message: format!( - "etag mismatch for part {}: expected {}, got {}", - completed.part_number, stored.etag, completed.etag - ), - })); - } - _ => {} - } - } - - let mut payload = BytesMut::new(); - for completed in &parts { - let stored = &upload.parts[&completed.part_number]; - payload.extend_from_slice(&stored.data); - } - - let mut metadata = upload.metadata.clone(); - metadata.size = Some(payload.len()); - - (metadata, payload.freeze()) - }; - - let expires_at = metadata.time_expires; - let entry = StoreEntry::Object(metadata, payload); - let size = entry.stored_size(); - self.store.lock().unwrap().insert(id.clone(), entry); - self.change_stream - .commit_write(id, size as u64, expires_at) - .await; - - self.multipart_store.lock().unwrap().remove(&key); - - Ok(None) - } -} - /// Serialized length of `value`, or `0` if it cannot be serialized. fn json_len(value: &T) -> usize { serde_json::to_string(value).map_or(0, |json| json.len()) @@ -983,7 +765,6 @@ impl Entry { #[cfg(test)] mod tests { use futures_util::StreamExt; - use std::num::NonZeroU32; use std::time::Duration; #[cfg(feature = "storage-cogs")] @@ -1538,336 +1319,6 @@ mod tests { ); } - #[tokio::test] - async fn multipart_single_part() { - let backend = InMemoryBackend::new("test"); - let id = make_id(); - let metadata = Metadata { - content_type: "text/plain".into(), - expiration_policy: ExpirationPolicy::TimeToIdle(Duration::from_hours(1)), - origin: Some("203.0.113.42".into()), - custom: [("foo".into(), "bar".into())].into(), - ..Default::default() - }; - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let data = b"hello, multipart world!"; - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - data.len() as u64, - None, - stream::single(data.to_vec()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none(), "expected no error on complete"); - - let (meta, _, body) = backend - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let payload = stream::read_to_vec(body).await.unwrap(); - assert_eq!(payload, data); - assert_eq!(meta.content_type, "text/plain".to_string()); - assert_eq!( - meta.expiration_policy, - ExpirationPolicy::TimeToIdle(Duration::from_hours(1)) - ); - assert_eq!(meta.origin, Some("203.0.113.42".into())); - assert_eq!(meta.custom, [("foo".into(), "bar".into())].into()); - } - - #[tokio::test] - async fn multipart_multiple_parts() { - let backend = InMemoryBackend::new("test"); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let part1 = b"aaaa".to_vec(); - let part2 = b"bbbb".to_vec(); - let part3 = b"cc".to_vec(); - - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - part1.len() as u64, - None, - stream::single(part1.clone()), - ) - .await - .unwrap(); - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - part2.len() as u64, - None, - stream::single(part2.clone()), - ) - .await - .unwrap(); - let etag3 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(3).unwrap(), - part3.len() as u64, - None, - stream::single(part3.clone()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![ - CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag1, - }, - CompletedPart { - part_number: NonZeroU32::new(2).unwrap(), - etag: etag2, - }, - CompletedPart { - part_number: NonZeroU32::new(3).unwrap(), - etag: etag3, - }, - ], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none()); - - let (_, _, body) = backend - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let payload = stream::read_to_vec(body).await.unwrap(); - assert_eq!(payload, b"aaaabbbbcc"); - } - - #[tokio::test] - async fn multipart_list_parts() { - let backend = InMemoryBackend::new("test"); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 3, - None, - stream::single(b"aaa".to_vec()), - ) - .await - .unwrap(); - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - 3, - None, - stream::single(b"bbb".to_vec()), - ) - .await - .unwrap(); - - let list = backend - .list_parts(&id, &upload_id, None, None) - .await - .unwrap(); - assert_eq!(list.parts.len(), 2); - assert_eq!(list.parts[0].part_number.get(), 1); - assert_eq!(list.parts[0].etag, etag1); - assert_eq!(list.parts[0].size, 3); - assert_eq!(list.parts[1].part_number.get(), 2); - assert_eq!(list.parts[1].etag, etag2); - assert_eq!(list.parts[1].size, 3); - - // Pagination - let page1 = backend - .list_parts(&id, &upload_id, Some(1), None) - .await - .unwrap(); - assert_eq!(page1.parts.len(), 1); - assert_eq!(page1.parts[0].part_number.get(), 1); - assert!(page1.is_truncated); - assert!(page1.next_part_number_marker.is_some()); - - let page2 = backend - .list_parts(&id, &upload_id, Some(1), page1.next_part_number_marker) - .await - .unwrap(); - assert_eq!(page2.parts.len(), 1); - assert_eq!(page2.parts[0].part_number.get(), 2); - - backend.abort_multipart(&id, &upload_id).await.unwrap(); - } - - #[tokio::test] - async fn multipart_abort() { - let backend = InMemoryBackend::new("test"); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await - .unwrap(); - - backend.abort_multipart(&id, &upload_id).await.unwrap(); - - let result = backend - .get_object(&id, Timestamp::now(), None) - .await - .unwrap(); - assert!(result.is_none(), "object should not exist after abort"); - } - - #[tokio::test] - async fn multipart_invalid_etag() { - let backend = InMemoryBackend::new("test"); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: "wrong-etag".into(), - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_some(), "expected error for bad etag"); - assert_eq!(result.unwrap().code, "InvalidPart"); - - // Upload must survive a failed complete so the client can retry. - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none(), "retry with correct etag should succeed"); - } - - #[tokio::test] - async fn multipart_missing_part() { - let backend = InMemoryBackend::new("test"); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(99).unwrap(), - etag: "whatever".into(), - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_some(), "expected error for missing part"); - assert_eq!(result.unwrap().code, "InvalidPart"); - - // Upload must survive a failed complete so the client can retry. - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none(), "retry with correct part should succeed"); - } - #[cfg(feature = "storage-cogs")] fn backend_with_change_stream() -> (InMemoryBackend, DummyProducer) { use crate::change_stream::CostTrackerStreamConfig; diff --git a/objectstore-service/src/backend/local_fs.rs b/objectstore-service/src/backend/local_fs.rs index f38a6cf9..f8dcdbe3 100644 --- a/objectstore-service/src/backend/local_fs.rs +++ b/objectstore-service/src/backend/local_fs.rs @@ -22,7 +22,6 @@ use std::num::NonZeroU64; use std::path::{Path, PathBuf}; use std::pin::pin; use std::sync::Arc; -use std::time::SystemTime; use futures_util::StreamExt; use objectstore_types::metadata::Metadata; @@ -38,18 +37,13 @@ use tokio_util::io::{ReaderStream, StreamReader}; use uuid::Uuid; use crate::backend::common::{ - self, Backend, DeleteResponse, ExpiryUpdate, GetResponse, MultipartUploadBackend, PutResponse, - SetExpiryResponse, + self, Backend, DeleteResponse, ExpiryUpdate, GetResponse, PutResponse, SetExpiryResponse, }; use crate::change_stream::{ ChangeStream, ChangeStreamFactory, CostTrackerStreamConfig, flush_change_stream, }; use crate::error::{Error, ErrorKind, Result, ResultExt as _}; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, Part, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session}; use crate::stream::{self, ClientStream}; @@ -153,10 +147,6 @@ impl Backend for LocalFsBackend { "local-fs" } - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - Ok(self) - } - #[tracing::instrument(level = "debug", fields(?id), skip_all)] async fn put_object( &self, @@ -419,353 +409,6 @@ impl Backend for LocalFsBackend { } } -impl LocalFsBackend { - fn multipart_dir(&self, id: &ObjectId, upload_id: &UploadId) -> PathBuf { - self.path - .join("__multipart__") - .join(id.as_storage_path().to_string()) - .join(upload_id.as_str()) - } -} - -#[async_trait::async_trait] -impl MultipartUploadBackend for LocalFsBackend { - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - self.change_stream - .begin_write(id, metadata.time_expires) - .await?; - let upload_id = UploadId::new(Uuid::now_v7().to_string())?; - let dir = self.multipart_dir(id, &upload_id); - create_directories(&dir).await.context( - ErrorKind::BackendFailure, - "creating local-fs multipart upload", - )?; - - let meta_path = dir.join("metadata.json"); - let metadata_json = serde_json::to_string(metadata) - .context(ErrorKind::Internal, "encoding local-fs multipart metadata")?; - let mut file = OpenOptions::from(file_options()) - .create(true) - .truncate(true) - .write(true) - .open(meta_path) - .await - .context( - ErrorKind::BackendFailure, - "creating local-fs multipart metadata", - )?; - file.write_all(metadata_json.as_bytes()).await.context( - ErrorKind::BackendFailure, - "writing local-fs multipart metadata", - )?; - file.flush().await.context( - ErrorKind::BackendFailure, - "flushing local-fs multipart metadata", - )?; - - Ok(upload_id) - } - - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - _content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - let dir = self.multipart_dir(id, upload_id); - if !tokio::fs::try_exists(&dir).await.context( - ErrorKind::BackendFailure, - "checking local-fs multipart upload", - )? { - return Err(Error::new( - ErrorKind::BackendFailure, - "local-fs multipart upload not found", - )); - } - - let etag = format!("\"etag-{part_number}-{content_length}\""); - - let header = serde_json::json!({ - "etag": etag, - "uploaded_at": SystemTime::now(), - "size": content_length, - }); - let header_line = serde_json::to_string(&header) - .context(ErrorKind::Internal, "encoding local-fs part header")?; - - let part_path = dir.join(format!("{part_number}.part")); - let file = OpenOptions::from(file_options()) - .create(true) - .write(true) - .truncate(true) - .open(part_path) - .await - .context( - ErrorKind::BackendFailure, - "opening local-fs multipart part for writing", - )?; - - let mut reader = pin!(StreamReader::new(body)); - let mut writer = BufWriter::new(file); - writer - .write_all(header_line.as_bytes()) - .await - .context(ErrorKind::BackendFailure, "writing local-fs part header")?; - writer - .write_all(b"\n") - .await - .context(ErrorKind::BackendFailure, "writing local-fs part header")?; - - let _bytes_copied = tokio::io::copy(&mut reader, &mut writer) - .await - .map_err(|e| match stream::unpack_client_error(&e) { - Some(ce) => Error::from(ce), - None => Error::with_context( - ErrorKind::BackendFailure, - "writing local-fs multipart part payload", - e, - ), - })?; - - // TODO: validate bytes_copied against content_length and return a BadRequest-style - // error. Needs a service-layer error variant that maps to HTTP 400 without abusing - // ClientError (which is meant for stream errors). - - writer.flush().await.context( - ErrorKind::BackendFailure, - "flushing local-fs multipart part", - )?; - let file = writer.into_inner(); - file.sync_data() - .await - .context(ErrorKind::BackendFailure, "syncing local-fs multipart part")?; - drop(file); - - Ok(etag) - } - - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - let dir = self.multipart_dir(id, upload_id); - if !tokio::fs::try_exists(&dir).await.context( - ErrorKind::BackendFailure, - "checking local-fs multipart upload", - )? { - return Err(Error::new( - ErrorKind::BackendFailure, - "local-fs multipart upload not found", - )); - } - - let mut entries = tokio::fs::read_dir(&dir).await.context( - ErrorKind::BackendFailure, - "listing local-fs multipart parts", - )?; - let mut parts = Vec::new(); - - while let Some(entry) = entries.next_entry().await.context( - ErrorKind::BackendFailure, - "listing local-fs multipart parts", - )? { - let name = entry.file_name(); - let name_str = name.to_string_lossy(); - let Some(pn_str) = name_str.strip_suffix(".part") else { - continue; - }; - let Ok(pn) = pn_str.parse::() else { - continue; - }; - - if part_number_marker.is_some_and(|marker| pn <= marker) { - continue; - } - - let file = tokio::fs::File::open(entry.path()) - .await - .context(ErrorKind::BackendFailure, "opening local-fs multipart part")?; - let mut reader = BufReader::new(file); - let mut header_line = String::new(); - reader - .read_line(&mut header_line) - .await - .context(ErrorKind::BackendFailure, "reading local-fs part header")?; - let header: serde_json::Value = serde_json::from_str(header_line.trim_end()) - .context(ErrorKind::CorruptData, "decoding local-fs part header")?; - - parts.push(Part { - part_number: pn, - etag: header["etag"].as_str().unwrap_or("").to_string(), - last_modified: serde_json::from_value(header["uploaded_at"].clone()) - .unwrap_or(SystemTime::UNIX_EPOCH), - size: header["size"].as_u64().unwrap_or(0), - }); - } - - parts.sort_by_key(|p| p.part_number); - - let max = max_parts.unwrap_or(u32::MAX) as usize; - let is_truncated = parts.len() > max; - parts.truncate(max); - - let next_part_number_marker = if is_truncated { - parts.last().map(|p| p.part_number) - } else { - None - }; - - Ok(ListPartsResponse { - parts, - is_truncated, - next_part_number_marker, - }) - } - - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - let dir = self.multipart_dir(id, upload_id); - if tokio::fs::try_exists(&dir).await.context( - ErrorKind::BackendFailure, - "checking local-fs multipart upload", - )? { - tokio::fs::remove_dir_all(dir).await.context( - ErrorKind::BackendFailure, - "removing local-fs multipart upload", - )?; - } - Ok(()) - } - - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - _access_time: Timestamp, - ) -> Result { - let dir = self.multipart_dir(id, upload_id); - if !tokio::fs::try_exists(&dir).await.context( - ErrorKind::BackendFailure, - "checking local-fs multipart upload", - )? { - return Err(Error::new( - ErrorKind::BackendFailure, - "local-fs multipart upload not found", - )); - } - - // Read metadata - let meta_path = dir.join("metadata.json"); - let meta_bytes = tokio::fs::read(&meta_path).await.context( - ErrorKind::BackendFailure, - "reading local-fs multipart metadata", - )?; - let metadata: Metadata = serde_json::from_slice(&meta_bytes).context( - ErrorKind::CorruptData, - "decoding local-fs multipart metadata", - )?; - - // TODO: validate that parts are in ascending part_number order and reject with - // InvalidPartOrder if not (matches S3/GCS behavior). Needs a proper client error variant. - - // Validate all parts (headers only) before writing anything - for completed in &parts { - let part_path = dir.join(format!("{}.part", completed.part_number)); - if !tokio::fs::try_exists(&part_path).await.context( - ErrorKind::BackendFailure, - "checking local-fs multipart part", - )? { - return Ok(Some(crate::multipart::CompleteMultipartError { - code: "InvalidPart".into(), - message: format!("part number {} was not uploaded", completed.part_number), - })); - } - - let file = tokio::fs::File::open(&part_path) - .await - .context(ErrorKind::BackendFailure, "opening local-fs multipart part")?; - let mut reader = BufReader::new(file); - let mut header_line = String::new(); - reader - .read_line(&mut header_line) - .await - .context(ErrorKind::BackendFailure, "reading local-fs part header")?; - let header: serde_json::Value = serde_json::from_str(header_line.trim_end()) - .context(ErrorKind::CorruptData, "decoding local-fs part header")?; - - let stored_etag = header["etag"].as_str().unwrap_or(""); - if stored_etag != completed.etag { - return Ok(Some(crate::multipart::CompleteMultipartError { - code: "InvalidPart".into(), - message: format!( - "etag mismatch for part {}: expected {}, got {}", - completed.part_number, stored_etag, completed.etag - ), - })); - } - } - - // Assemble the parts into a draft before publishing the object. - let path = self.path(id); - create_directories(path.parent().unwrap()).await.context( - ErrorKind::BackendFailure, - "creating local-fs object directory", - )?; - let mut draft = Draft::create(&path, &metadata).await?; - - let mut payload_size = 0; - for completed in &parts { - let part_path = dir.join(format!("{}.part", completed.part_number)); - let file = tokio::fs::File::open(&part_path) - .await - .context(ErrorKind::BackendFailure, "opening local-fs multipart part")?; - let mut reader = BufReader::new(file); - let mut header_line = String::new(); - reader - .read_line(&mut header_line) - .await - .context(ErrorKind::BackendFailure, "reading local-fs part header")?; - payload_size += tokio::io::copy(&mut reader, draft.writer()).await.context( - ErrorKind::BackendFailure, - "assembling local-fs object payload", - )?; - } - - let stored_size = draft.preamble_len() + payload_size; - - draft.prepare().await?; - let guard = self.locks.acquire(id).await?; - draft.publish().await?; - drop(guard); - - self.change_stream - .commit_write(id, stored_size, metadata.time_expires) - .await; - - // Clean up multipart state - tokio::fs::remove_dir_all(dir).await.context( - ErrorKind::BackendFailure, - "removing local-fs multipart upload", - )?; - - Ok(None) - } -} - // Must be lower than the tokio runtime `max_blocking_threads` setting. const MAX_BLOCKING_LOCK_WAITERS: usize = 256; @@ -1119,7 +762,6 @@ fn create_tempfile(parent: &Path) -> io::Result { #[cfg(test)] mod tests { use std::collections::HashMap; - use std::num::NonZeroU32; use std::sync::Arc; use std::time::Duration; @@ -1778,205 +1420,6 @@ mod tests { .count() } - #[tokio::test] - async fn multipart_single_part() { - let (_tempdir, backend) = make_backend(); - let id = make_id(); - let metadata = Metadata { - content_type: "text/plain".into(), - expiration_policy: ExpirationPolicy::TimeToIdle(Duration::from_hours(1)), - origin: Some("203.0.113.42".into()), - custom: [("foo".into(), "bar".into())].into(), - ..Default::default() - }; - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let data = b"hello, multipart world!"; - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - data.len() as u64, - None, - stream::single(data.to_vec()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none(), "expected no error on complete"); - - let (meta, _, body) = backend - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let payload: BytesMut = body.try_collect().await.unwrap(); - assert_eq!(payload.as_ref(), data); - assert_eq!(meta.content_type, "text/plain".to_string()); - assert_eq!( - meta.expiration_policy, - ExpirationPolicy::TimeToIdle(Duration::from_hours(1)) - ); - assert_eq!(meta.origin, Some("203.0.113.42".into())); - assert_eq!(meta.custom, [("foo".into(), "bar".into())].into()); - } - - #[tokio::test] - async fn multipart_multiple_parts() { - let (_tempdir, backend) = make_backend(); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let part1 = b"aaaa".to_vec(); - let part2 = b"bbbb".to_vec(); - let part3 = b"cc".to_vec(); - - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - part1.len() as u64, - None, - stream::single(part1.clone()), - ) - .await - .unwrap(); - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - part2.len() as u64, - None, - stream::single(part2.clone()), - ) - .await - .unwrap(); - let etag3 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(3).unwrap(), - part3.len() as u64, - None, - stream::single(part3.clone()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![ - CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag1, - }, - CompletedPart { - part_number: NonZeroU32::new(2).unwrap(), - etag: etag2, - }, - CompletedPart { - part_number: NonZeroU32::new(3).unwrap(), - etag: etag3, - }, - ], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none()); - - let (_, _, body) = backend - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let payload: BytesMut = body.try_collect().await.unwrap(); - assert_eq!(payload.as_ref(), b"aaaabbbbcc"); - } - - #[tokio::test] - async fn multipart_list_parts() { - let (_tempdir, backend) = make_backend(); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag1 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 3, - None, - stream::single(b"aaa".to_vec()), - ) - .await - .unwrap(); - let etag2 = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - 3, - None, - stream::single(b"bbb".to_vec()), - ) - .await - .unwrap(); - - let list = backend - .list_parts(&id, &upload_id, None, None) - .await - .unwrap(); - assert_eq!(list.parts.len(), 2); - assert_eq!(list.parts[0].part_number.get(), 1); - assert_eq!(list.parts[0].etag, etag1); - assert_eq!(list.parts[0].size, 3); - assert_eq!(list.parts[1].part_number.get(), 2); - assert_eq!(list.parts[1].etag, etag2); - assert_eq!(list.parts[1].size, 3); - - // Pagination - let page1 = backend - .list_parts(&id, &upload_id, Some(1), None) - .await - .unwrap(); - assert_eq!(page1.parts.len(), 1); - assert_eq!(page1.parts[0].part_number.get(), 1); - assert!(page1.is_truncated); - assert!(page1.next_part_number_marker.is_some()); - - let page2 = backend - .list_parts(&id, &upload_id, Some(1), page1.next_part_number_marker) - .await - .unwrap(); - assert_eq!(page2.parts.len(), 1); - assert_eq!(page2.parts[0].part_number.get(), 2); - - backend.abort_multipart(&id, &upload_id).await.unwrap(); - } - #[tokio::test] async fn get_object_range_bounded() { let (_tempdir, backend) = make_backend(); @@ -2099,137 +1542,6 @@ mod tests { } } - #[tokio::test] - async fn multipart_abort() { - let (_tempdir, backend) = make_backend(); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await - .unwrap(); - - backend.abort_multipart(&id, &upload_id).await.unwrap(); - - let result = backend - .get_object(&id, Timestamp::now(), None) - .await - .unwrap(); - assert!(result.is_none(), "object should not exist after abort"); - } - - #[tokio::test] - async fn multipart_invalid_etag() { - let (_tempdir, backend) = make_backend(); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: "wrong-etag".into(), - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_some(), "expected error for bad etag"); - assert_eq!(result.unwrap().code, "InvalidPart"); - - // Upload must survive a failed complete so the client can retry. - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none(), "retry with correct etag should succeed"); - } - - #[tokio::test] - async fn multipart_missing_part() { - let (_tempdir, backend) = make_backend(); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag = backend - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - 5, - None, - stream::single(b"hello".to_vec()), - ) - .await - .unwrap(); - - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(99).unwrap(), - etag: "whatever".into(), - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_some(), "expected error for missing part"); - assert_eq!(result.unwrap().code, "InvalidPart"); - - // Upload must survive a failed complete so the client can retry. - let result = backend - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(result.is_none(), "retry with correct part should succeed"); - } - #[cfg(feature = "storage-cogs")] #[tokio::test] async fn change_stream_reports_size_written_to_disk() { @@ -2319,57 +1631,6 @@ mod tests { assert!(records[0].expiration_time.is_some()); } - #[cfg(feature = "storage-cogs")] - #[tokio::test] - async fn change_stream_reports_assembled_multipart_size() { - let (tempdir, backend, producer) = make_backend_with_change_stream(); - let id = make_id(); - let metadata = Metadata::default(); - - let upload_id = backend.initiate_multipart(&id, &metadata).await.unwrap(); - - let mut completed = Vec::new(); - for (number, payload) in [(1u32, b"aaaa".to_vec()), (2, b"bbbb".to_vec())] { - let part_number = NonZeroU32::new(number).unwrap(); - let etag = backend - .upload_part( - &id, - &upload_id, - part_number, - payload.len() as u64, - None, - stream::single(payload), - ) - .await - .unwrap(); - completed.push(CompletedPart { part_number, etag }); - } - - // Uploading parts is intermediate state, so nothing is reported until completion. - assert!(producer.records().is_empty()); - - assert!( - backend - .complete_multipart(&id, &upload_id, completed, Timestamp::now()) - .await - .unwrap() - .is_none() - ); - - let file = tokio::fs::read(tempdir.path().join(id.as_storage_path().to_string())) - .await - .unwrap(); - - let records = producer.records(); - assert_eq!(records.len(), 1); - assert_eq!(records[0].op_type, OpType::Write); - assert_eq!( - records[0].size, - Some(file.len() as u64), - "the assembled object counts its metadata header and every part payload" - ); - } - #[cfg(feature = "storage-cogs")] #[tokio::test] async fn change_stream_reports_deletes_on_success() { diff --git a/objectstore-service/src/backend/mod.rs b/objectstore-service/src/backend/mod.rs index a0b6b81d..71a63d75 100644 --- a/objectstore-service/src/backend/mod.rs +++ b/objectstore-service/src/backend/mod.rs @@ -123,10 +123,10 @@ async fn hv_from_config( /// Configuration for the long-term backend in a [`tiered::TieredStorageConfig`]. /// -/// Only backends that implement [`common::MultipartUploadBackend`] are valid here. +/// Supports GCS and local filesystem storage. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(tag = "type", rename_all = "lowercase")] -pub enum MultipartUploadStorageConfig { +pub enum LongTermStorageConfig { /// Local filesystem storage backend (type `"filesystem"`). FileSystem(local_fs::FileSystemConfig), @@ -136,15 +136,13 @@ pub enum MultipartUploadStorageConfig { Gcs(gcs::GcsConfig), } -/// Constructs a type-erased [`common::MultipartUploadBackend`] from the given config. +/// Constructs a type-erased [`common::Backend`] from the given config. async fn lt_from_config( - config: MultipartUploadStorageConfig, + config: LongTermStorageConfig, streams: &ChangeStreamFactory, -) -> Result> { +) -> Result> { Ok(match config { - MultipartUploadStorageConfig::FileSystem(c) => { - Box::new(local_fs::LocalFsBackend::new(c, streams)) - } - MultipartUploadStorageConfig::Gcs(c) => Box::new(gcs::GcsBackend::new(c, streams).await?), + LongTermStorageConfig::FileSystem(c) => Box::new(local_fs::LocalFsBackend::new(c, streams)), + LongTermStorageConfig::Gcs(c) => Box::new(gcs::GcsBackend::new(c, streams).await?), }) } diff --git a/objectstore-service/src/backend/testing.rs b/objectstore-service/src/backend/testing.rs index 4262c2f4..dbfa6913 100644 --- a/objectstore-service/src/backend/testing.rs +++ b/objectstore-service/src/backend/testing.rs @@ -48,16 +48,12 @@ use objectstore_types::resumable::UploadProgress; use crate::backend::common::{ Backend, DeleteResponse, ExpiryUpdate, GetResponse, HighVolumeBackend, MetadataResponse, - MultipartUploadBackend, PutResponse, SetExpiryResponse, TieredGet, TieredMetadata, - TieredUpdate, TieredWrite, Tombstone, + PutResponse, SetExpiryResponse, TieredGet, TieredMetadata, TieredUpdate, TieredWrite, + Tombstone, }; use crate::backend::in_memory::InMemoryBackend; use crate::error::Result; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session}; use crate::stream::ClientStream; @@ -247,80 +243,6 @@ pub trait Hooks: fmt::Debug + Send + Sync + 'static { .await } - // --- MultipartUploadBackend methods --- - - /// Intercepts [`MultipartUploadBackend::initiate_multipart`]. Default delegates to `inner`. - async fn initiate_multipart( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - inner.initiate_multipart(id, metadata).await - } - - /// Intercepts [`MultipartUploadBackend::upload_part`]. Default delegates to `inner`. - #[allow(clippy::too_many_arguments)] - async fn upload_part( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - inner - .upload_part( - id, - upload_id, - part_number, - content_length, - content_md5, - body, - ) - .await - } - - /// Intercepts [`MultipartUploadBackend::list_parts`]. Default delegates to `inner`. - async fn list_parts( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - inner - .list_parts(id, upload_id, max_parts, part_number_marker) - .await - } - - /// Intercepts [`MultipartUploadBackend::abort_multipart`]. Default delegates to `inner`. - async fn abort_multipart( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - inner.abort_multipart(id, upload_id).await - } - - /// Intercepts [`MultipartUploadBackend::complete_multipart`]. Default delegates to `inner`. - async fn complete_multipart( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - inner - .complete_multipart(id, upload_id, parts, access_time) - .await - } - // --- Resumable upload methods --- /// Intercepts [`Backend::create_upload_session`]. Default delegates to `inner`. @@ -411,10 +333,6 @@ impl Backend for TestBackend { self.hooks.upload_granularity(&self.inner) } - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - Ok(self) - } - async fn put_object( &self, id: &ObjectId, @@ -592,70 +510,3 @@ impl HighVolumeBackend for TestBackend { .await } } - -#[async_trait::async_trait] -impl MultipartUploadBackend for TestBackend { - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - self.hooks - .initiate_multipart(&self.inner, id, metadata) - .await - } - - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - self.hooks - .upload_part( - &self.inner, - id, - upload_id, - part_number, - content_length, - content_md5, - body, - ) - .await - } - - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - self.hooks - .list_parts(&self.inner, id, upload_id, max_parts, part_number_marker) - .await - } - - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - self.hooks.abort_multipart(&self.inner, id, upload_id).await - } - - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - self.hooks - .complete_multipart(&self.inner, id, upload_id, parts, access_time) - .await - } -} diff --git a/objectstore-service/src/backend/tiered.rs b/objectstore-service/src/backend/tiered.rs index ed383cf4..4984c424 100644 --- a/objectstore-service/src/backend/tiered.rs +++ b/objectstore-service/src/backend/tiered.rs @@ -111,9 +111,8 @@ use std::num::NonZeroU64; use std::sync::Arc; use std::sync::atomic::Ordering; -use std::time::{Duration, SystemTime}; +use std::time::Duration; -use base64::Engine as _; use bytes::Bytes; use futures_util::StreamExt; use objectstore_metrics::TimerGuard; @@ -127,16 +126,12 @@ use serde::{Deserialize, Serialize}; use crate::backend::changelog::{Change, ChangeGuard, ChangeLog, ChangeManager, ChangePhase}; use crate::backend::common::{ Backend, DeleteResponse, ExpiryTarget, ExpiryUpdate, GetResponse, HighVolumeBackend, - MetadataResponse, MultipartUploadBackend, PutResponse, SetExpiryResponse, TieredGet, - TieredMetadata, TieredUpdate, TieredWrite, Tombstone, + MetadataResponse, PutResponse, SetExpiryResponse, TieredGet, TieredMetadata, TieredUpdate, + TieredWrite, Tombstone, }; -use crate::backend::{HighVolumeStorageConfig, MultipartUploadStorageConfig}; -use crate::error::{Error, ErrorKind, Result, ResultExt as _}; +use crate::backend::{HighVolumeStorageConfig, LongTermStorageConfig}; +use crate::error::{ErrorKind, Result, ResultExt as _}; use crate::id::ObjectId; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::{BackendToken, Session}; use crate::stream::{ClientStream, SizedPeek, counting_stream}; @@ -146,12 +141,6 @@ const BACKEND_SIZE_THRESHOLD: usize = 1024 * 1024; // 1 MiB /// Fixed lifetime of an ongoing upload marker. const RESUMABLE_UPLOAD_TTL: Duration = Duration::from_hours(5 * 24); -/// Amount of time for which a `Change` generated by a `complete_multipart` operation is kept in the `Assembling` -/// state before becoming eligible for cleanup by the `ChangeLog` recovery process. -/// This allows the client to retry the `complete_multipart` operation upon any failures for at least this long, -/// avoiding scenarios where the `ChangeLog` recovery would race to delete the assembled LT blob. -const MULTIPART_COMPLETE_CLEANUP_DELAY: Duration = Duration::from_hours(24); - /// Creates a new [`ObjectId`] with the same context but a unique revision key. /// /// The new key has the format `{original_key}/{uuid_v7}`, producing a distinct @@ -193,8 +182,8 @@ pub struct TieredStorageConfig { pub high_volume: HighVolumeStorageConfig, /// Backend for large, long-term objects. /// - /// Must be a backend that implements [`MultipartUploadBackend`]. - pub long_term: MultipartUploadStorageConfig, + /// Supports GCS and local filesystem storage. + pub long_term: LongTermStorageConfig, } /// Two-tier storage backend that routes objects by size. @@ -256,7 +245,7 @@ impl TieredStorage { /// Creates a new `TieredStorage` with the given backends and change log. pub fn new( high_volume: Box, - long_term: Box, + long_term: Box, changelog: Box, ) -> Self { let inner = ChangeManager::new(high_volume, long_term, changelog); @@ -272,12 +261,6 @@ impl TieredStorage { self.inner.clone().record(change).await } - /// Records the change to the log in the `Assembling` phase, and returns a guard that does - /// nothing on drop unless advanced. - async fn record_assembling(&self, change: Change) -> Result { - self.inner.clone().record_assembling(change).await - } - /// Checks that the upload is still authorized before contacting LT. async fn check_upload_marker(&self, revision: &ObjectId) -> Result<()> { if !self @@ -347,7 +330,6 @@ impl TieredStorage { id: id.clone(), new: None, old: Some(target.clone()), - cleanup_after: None, }) .await?; @@ -396,7 +378,6 @@ impl TieredStorage { id: id.clone(), new: Some(new.clone()), old: current.clone(), - cleanup_after: None, }) .await?; @@ -466,10 +447,6 @@ impl Backend for TieredStorage { self.inner.long_term.upload_granularity() } - fn as_multipart_upload_backend(&self) -> Result<&dyn MultipartUploadBackend> { - Ok(self) - } - #[tracing::instrument(level = "debug", fields(?id, upload_length), skip_all)] async fn create_upload_session( &self, @@ -598,7 +575,6 @@ impl Backend for TieredStorage { id: id.clone(), new: Some(revision.clone()), old: current.clone(), - cleanup_after: None, }) .await?; @@ -856,7 +832,6 @@ impl Backend for TieredStorage { id: id.clone(), new: None, old: Some(tombstone.target.clone()), - cleanup_after: None, }) .await?; guard.advance(ChangePhase::Written); @@ -936,329 +911,11 @@ fn align_expiry(mut metadata: Metadata, tombstone: &Tombstone) -> Metadata { metadata } -/// The multipart upload state for TieredStorage. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -struct TieredUploadId { - revision: String, - upload_id: UploadId, -} - -impl TryInto for TieredUploadId { - type Error = Error; - - fn try_into(self) -> Result { - let json = - serde_json::to_vec(&self).context(ErrorKind::Internal, "encoding tiered upload ID")?; - Ok(UploadId::new( - base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(json), - )?) - } -} - -impl TryFrom<&UploadId> for TieredUploadId { - type Error = Error; - - fn try_from(value: &UploadId) -> Result { - let json = base64::engine::general_purpose::URL_SAFE_NO_PAD - .decode(value.as_bytes()) - .kind(ErrorKind::InvalidUploadId)?; - serde_json::from_slice(&json).kind(ErrorKind::InvalidUploadId) - } -} - -#[async_trait::async_trait] -impl MultipartUploadBackend for TieredStorage { - #[tracing::instrument(level = "debug", fields(?id), skip_all)] - async fn initiate_multipart( - &self, - id: &ObjectId, - metadata: &Metadata, - ) -> Result { - let timer = objectstore_metrics::timer!( - "multipart.initiate.latency", - usecase = id.usecase().to_string(), - backend_choice = BackendChoice::LongTerm.as_str(), - backend_type = self.backend_type(&BackendChoice::LongTerm), - ); - let physical = new_long_term_revision(id); - - let upload_id = self - .inner - .long_term - .initiate_multipart(&physical, metadata) - .await?; - - let id = TieredUploadId { - revision: physical.key, - upload_id, - }; - let id = id.try_into()?; - - timer.record(); - Ok(id) - } - - #[tracing::instrument(level = "debug", fields(?id, part_number, content_length), skip_all)] - async fn upload_part( - &self, - id: &ObjectId, - upload_id: &UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option<&str>, - body: ClientStream, - ) -> Result { - let timer = objectstore_metrics::timer!( - "multipart.upload_part.latency", - usecase = id.usecase().to_string(), - backend_choice = BackendChoice::LongTerm.as_str(), - backend_type = self.backend_type(&BackendChoice::LongTerm), - ); - let tiered: TieredUploadId = upload_id.try_into()?; - - let physical = ObjectId { - context: id.context.clone(), - key: tiered.revision, - }; - - let etag = self - .inner - .long_term - .upload_part( - &physical, - &tiered.upload_id, - part_number, - content_length, - content_md5, - body, - ) - .await?; - - timer.record(); - objectstore_metrics::record!( - "multipart.upload_part.size" = content_length, - usecase = id.usecase().to_string(), - ); - - Ok(etag) - } - - #[tracing::instrument(level = "debug", skip(self, upload_id))] - async fn list_parts( - &self, - id: &ObjectId, - upload_id: &UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - let timer = objectstore_metrics::timer!( - "multipart.list_parts.latency", - usecase = id.usecase().to_string(), - backend_choice = BackendChoice::LongTerm.as_str(), - backend_type = self.backend_type(&BackendChoice::LongTerm), - ); - let tiered: TieredUploadId = upload_id.try_into()?; - - let physical = ObjectId { - context: id.context.clone(), - key: tiered.revision, - }; - - let response = self - .inner - .long_term - .list_parts(&physical, &tiered.upload_id, max_parts, part_number_marker) - .await?; - - timer.record(); - Ok(response) - } - - #[tracing::instrument(level = "debug", fields(?id), skip_all)] - async fn abort_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - ) -> Result { - let timer = objectstore_metrics::timer!( - "multipart.abort.latency", - usecase = id.usecase().to_string(), - backend_choice = BackendChoice::LongTerm.as_str(), - backend_type = self.backend_type(&BackendChoice::LongTerm), - ); - let tiered: TieredUploadId = upload_id.try_into()?; - - let physical = ObjectId { - context: id.context.clone(), - key: tiered.revision, - }; - - let () = self - .inner - .long_term - .abort_multipart(&physical, &tiered.upload_id) - .await?; - - timer.record(); - Ok(()) - } - - #[tracing::instrument(level = "debug", fields(?id), skip_all)] - async fn complete_multipart( - &self, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - let timer = objectstore_metrics::timer!( - "multipart.complete.latency", - usecase = id.usecase().to_string(), - backend_choice = BackendChoice::LongTerm.as_str(), - backend_type = self.backend_type(&BackendChoice::LongTerm), - ); - let part_count = parts.len(); - let tiered: TieredUploadId = upload_id.try_into()?; - - let physical = ObjectId { - context: id.context.clone(), - key: tiered.revision, - }; - - // 1. Read current HV revision to establish the write precondition. - let current = match self - .inner - .high_volume - .get_tiered_metadata(id, access_time) - .await? - { - // Optimization: a previous attempt already finalized this revision and tombstone -- report success. - TieredMetadata::Tombstone(t) if t.target == physical => { - timer.record(); - return Ok(None); - } - TieredMetadata::Tombstone(t) => Some(t.target), - _ => None, - }; - - // Register a guard with cleanup deferred to now + `MULTIPART_COMPLETE_CLEANUP_DELAY`, - // so that the user has the chance to retry finalizing the upload in this timeframe. - let mut guard = self - .record_assembling(Change { - id: id.clone(), - new: Some(physical.clone()), - old: current.clone(), - cleanup_after: Some(SystemTime::now() + MULTIPART_COMPLETE_CLEANUP_DELAY), - }) - .await?; - - // 2. Complete the upload, creating the object at the given revision key. - let maybe_complete_multipart_err = match self - .inner - .long_term - .complete_multipart(&physical, &tiered.upload_id, parts, access_time) - .await - { - // The request went through but we got an error in the response body. - // Transparently proxy the error to the user. - Ok(error) => { - if error.is_some() { - return Ok(error); - } - None - } - // We got status 4xx/5xx, or a network error. - // Either way, `complete_multipart` might have been completed successfully, - // either now or in a previous attempt (in that case, that's a 404 and we indeed end up - // here). - // We cannot know if that's the case yet, so we continue to the next steps. - Err(err) => Some(err), - }; - - // 3. Retrieve the metadata of the object, which was determined at initiation time, to - // get its expiration deadline and size. - // - // This also serves as an existence check to understand if the LT revision was actually - // created successfully in this or a previous attempt, in which case we just need to - // finalize the tombstone. - let metadata = self - .inner - .long_term - .get_metadata(&physical, access_time) - .await; - - let metadata = match (metadata, maybe_complete_multipart_err) { - // The LT revision already exists, so we can continue to finalize the tombstone. - (Ok(Some(metadata)), _) => metadata, - // The LT revision doesn't exist, cannot proceed. - (Ok(None), Some(err)) => return Err(err), - // The `complete_multipart` succeeded, creating the object, but the `get_metadata` - // immediately after failed to find the object. This should never happen. - (Ok(None), None) => { - objectstore_log::error!( - id = ?id, - upload_id = ?upload_id, - physical = ?physical, - "complete_multipart call succeeded on long_term backend, but subsequent get_metadata found no object" - ); - return Err(Error::new( - ErrorKind::BackendFailure, - "tiered multipart object missing from long-term storage", - )); - } - // Failed to `get_metadata`, cannot proceed. - (Err(get_metadata_err), maybe_complete_multipart_err) => { - // Prefer the `complete_multipart_err`, as it's likely more informative. - // TODO(FS-358): convert this properly. Right now `ApiErrorResponse` will turn this into a 500, - // but we would actually want to transparently surface the original status (and message?) instead. - return Err(maybe_complete_multipart_err.unwrap_or(get_metadata_err)); - } - }; - - // 4. CAS commit: write tombstone only if HV state matches what we saw. - let tombstone = Tombstone { - target: physical.clone(), - time_expires: metadata.time_expires, - }; - let written = self - .inner - .high_volume - .compare_and_write( - id, - current.as_ref(), - TieredWrite::Tombstone(tombstone), - access_time, - ) - .await?; - - // Update guard and let it schedule cleanup in the background. - guard.advance(ChangePhase::compare_and_write(written)); - - timer.record(); - objectstore_metrics::record!( - "multipart.complete.part_count" = part_count as u64, - usecase = id.usecase().to_string(), - ); - if let Some(size) = metadata.size { - objectstore_metrics::record!( - "put.size" = size as u64, - usecase = id.usecase().to_string(), - backend_choice = BackendChoice::LongTerm.as_str(), - backend_type = self.backend_type(&BackendChoice::LongTerm), - upload_type = "multipart", - ); - } - - Ok(None) - } -} - #[cfg(test)] mod tests { - use std::num::{NonZeroU32, NonZeroU64}; + use std::num::NonZeroU64; use std::sync::Mutex as StdMutex; - use futures::lock::Mutex; use objectstore_types::metadata::{ExpirationPolicy, Metadata}; use objectstore_types::scope::{Scope, Scopes}; @@ -2761,7 +2418,6 @@ mod tests { id: make_id("object-key"), new: Some(make_id("cleanup-target")), old: None, - cleanup_after: None, }; // Build the guard inside a temporary runtime, then let the runtime drop @@ -2785,7 +2441,6 @@ mod tests { id: make_id("object-key"), new: None, old: None, - cleanup_after: None, }; let mut guard = storage.record_change(change).await.unwrap(); @@ -2878,763 +2533,4 @@ mod tests { "changelog entry not removed after cleanup" ); } - - // --- Multipart upload --- - - #[test] - fn multipart_upload_id_roundtrip() { - let id = TieredUploadId { - revision: "my-key/01924a6f-7e28-7b9a-9c1d-abcdef123456".into(), - upload_id: UploadId::new("upstream-upload-id-abc".into()).unwrap(), - }; - let encoded: UploadId = id.clone().try_into().unwrap(); - let decoded: TieredUploadId = (&encoded.clone()).try_into().unwrap(); - assert_eq!(decoded, id); - } - - #[test] - fn malformed_multipart_upload_ids_are_invalid_upload_ids() { - let invalid_base64 = UploadId::new("%%%".into()).unwrap(); - let malformed_json = UploadId::new("bm90IGpzb24".into()).unwrap(); - - for upload_id in [&invalid_base64, &malformed_json] { - let error = TieredUploadId::try_from(upload_id).unwrap_err(); - assert_eq!(error.kind(), ErrorKind::InvalidUploadId); - } - } - - #[tokio::test] - async fn multipart_single_part_roundtrip() { - let (storage, hv, lt, _) = make_tiered_storage(); - let id = make_id("mp-single"); - let metadata = Metadata { - content_type: "application/octet-stream".into(), - expiration_policy: ExpirationPolicy::TimeToLive(Duration::from_hours(1)), - time_expires: Some(Timestamp::now() + Duration::from_hours(1)), - ..Metadata::default() - }; - let payload = vec![0xABu8; 2 * 1024 * 1024]; // 2 MiB - - let upload_id = storage.initiate_multipart(&id, &metadata).await.unwrap(); - - let etag = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload.clone()), - ) - .await - .unwrap(); - - let error = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!( - error.is_none(), - "complete_multipart returned error: {error:?}" - ); - - // get_object should follow the tombstone and return the payload. - let (got_meta, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - assert_eq!(body, payload); - assert_eq!(got_meta.content_type, "application/octet-stream"); - - // HV should have a tombstone, LT should have the object at the physical key. - let tombstone = hv.get(&id).expect_tombstone(); - assert!( - tombstone.target.key().starts_with(id.key()), - "tombstone target should be a revision key" - ); - assert_eq!(tombstone.time_expires, metadata.time_expires); - assert_eq!( - lt.get(&tombstone.target).expect_object().0.time_expires, - tombstone.time_expires - ); - } - - #[tokio::test] - async fn multipart_upload() { - let (storage, _hv, _lt, _) = make_tiered_storage(); - let id = make_id("multipart"); - - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - let part1 = vec![0xAAu8; 512 * 1024]; - let part2 = vec![0xBBu8; 512 * 1024]; - let part3 = vec![0xCCu8; 512 * 1024]; - - let etag3 = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(3).unwrap(), - part3.len() as u64, - None, - stream::single(part3.clone()), - ) - .await - .unwrap(); - let etag2 = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - part2.len() as u64, - None, - stream::single(part2.clone()), - ) - .await - .unwrap(); - let etag1 = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - part1.len() as u64, - None, - stream::single(part1.clone()), - ) - .await - .unwrap(); - - let error = storage - .complete_multipart( - &id, - &upload_id, - vec![ - CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag1, - }, - CompletedPart { - part_number: NonZeroU32::new(2).unwrap(), - etag: etag2, - }, - CompletedPart { - part_number: NonZeroU32::new(3).unwrap(), - etag: etag3, - }, - ], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(error.is_none()); - - let (_, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - - let mut expected = Vec::new(); - expected.extend_from_slice(&part1); - expected.extend_from_slice(&part2); - expected.extend_from_slice(&part3); - assert_eq!(body, expected); - } - - #[tokio::test] - async fn multipart_abort() { - let (storage, hv, _lt, _) = make_tiered_storage(); - let id = make_id("mp-abort"); - - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - // Upload a part then abort. - let payload = vec![0xABu8; 100]; - storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload), - ) - .await - .unwrap(); - - storage.abort_multipart(&id, &upload_id).await.unwrap(); - - // No tombstone should have been written. - hv.get(&id).expect_not_found(); - - // The object should not be reachable. - assert!( - storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .is_none() - ); - } - - #[tokio::test] - async fn multipart_list_parts() { - let (storage, _hv, _lt, _) = make_tiered_storage(); - let id = make_id("mp-list"); - - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - let part1 = vec![0xAAu8; 100]; - let part2 = vec![0xBBu8; 200]; - storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - part1.len() as u64, - None, - stream::single(part1), - ) - .await - .unwrap(); - storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(2).unwrap(), - part2.len() as u64, - None, - stream::single(part2), - ) - .await - .unwrap(); - - let resp = storage - .list_parts(&id, &upload_id, None, None) - .await - .unwrap(); - assert_eq!(resp.parts.len(), 2); - assert_eq!(resp.parts[0].part_number.get(), 1); - assert_eq!(resp.parts[0].size, 100); - assert_eq!(resp.parts[1].part_number.get(), 2); - assert_eq!(resp.parts[1].size, 200); - } - - #[tokio::test] - async fn multipart_overwrites_existing_tombstone() { - let (storage, hv, lt, _) = make_tiered_storage(); - let id = make_id("mp-overwrite"); - - // Put a large object via the normal path. - let payload1 = vec![0xAAu8; 2 * 1024 * 1024]; - storage - .put_object( - &id, - &Metadata::default(), - stream::single(payload1), - Timestamp::now(), - ) - .await - .unwrap(); - let old_lt_id = hv.get(&id).expect_tombstone().target; - - // Overwrite via multipart. - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - let payload2 = vec![0xBBu8; 2 * 1024 * 1024]; - let etag = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload2.len() as u64, - None, - stream::single(payload2.clone()), - ) - .await - .unwrap(); - - // The multipart upload is not finalized, so the tombstone still points to the old - // revision. - let lt_id = hv.get(&id).expect_tombstone().target; - assert_eq!(old_lt_id, lt_id); - - let error = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await - .unwrap(); - assert!(error.is_none()); - - // Now the upload has been finalized, so the new tombstone points to the new revision. - let new_lt_id = hv.get(&id).expect_tombstone().target; - assert_ne!(old_lt_id, new_lt_id); - - // Wait for background cleanup. - storage.join().await; - - // Old revision should be cleaned up. - lt.get(&old_lt_id).expect_not_found(); - lt.get(&new_lt_id).expect_object(); - - // Assert the contents of the new revision. - let (_, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - assert_eq!(body, payload2); - } - - // --- Multipart completion failure handling (consistency, retries, delayed cleanup) --- - - /// Assembles the blob via `complete_multipart`, but returns an error to simulate a network - /// failure on the response path. Also fails `get_metadata` so the tiered layer cannot - /// recover by detecting the already-assembled blob. - #[derive(Debug)] - struct CompleteMultipartButReturnError; - - #[async_trait::async_trait] - impl Hooks for CompleteMultipartButReturnError { - async fn complete_multipart( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - inner - .complete_multipart(id, upload_id, parts, access_time) - .await - .unwrap(); - Err(Error::with_source( - ErrorKind::BackendFailure, - std::io::Error::new( - std::io::ErrorKind::TimedOut, - "simulated network error on complete_multipart", - ), - )) - } - - async fn get_metadata( - &self, - _inner: &InMemoryBackend, - _id: &ObjectId, - _access_time: Timestamp, - ) -> Result { - Err(Error::with_source( - ErrorKind::BackendFailure, - std::io::Error::new( - std::io::ErrorKind::TimedOut, - "simulated network error on get_metadata", - ), - )) - } - } - - /// `complete_multipart` on the inner LT backend assembles the blob successfully, but both - /// `complete_multipart` and `get_metadata` return errors, so the tiered layer cannot finalize. - /// After `MULTIPART_COMPLETE_CLEANUP_DELAY`, `ChangeLog` recovery deletes the orphaned blob. - #[tokio::test] - async fn cleans_up_orphan_after_failed_multipart_complete() { - let hv = InMemoryBackend::new("hv"); - let lt_inner = InMemoryBackend::new("lt"); - let log = InMemoryChangeLog::default(); - let storage = TieredStorage::new( - Box::new(hv.clone()), - Box::new(TestBackend::with_inner( - lt_inner.clone(), - CompleteMultipartButReturnError {}, - )), - Box::new(log.clone()), - ); - - let id = make_id("mp-orphan"); - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - let tiered_id: TieredUploadId = (&upload_id).try_into().unwrap(); - let physical = ObjectId { - context: id.context.clone(), - key: tiered_id.revision, - }; - - let payload = vec![0xABu8; 2 * 1024 * 1024]; - let etag = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload), - ) - .await - .unwrap(); - - let result = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await; - assert!(result.is_err()); - storage.join().await; - - // The LT blob is orphaned, and no cleanup has been performed (yet), due to the guard being - // dropped while in the `Assembling` state. - lt_inner.get(&physical).expect_object(); - hv.get(&id).expect_not_found(); - - // Simulate the passage of time and run recovery. - log.expire_all(); - let manager = ChangeManager::new( - Box::new(hv.clone()), - Box::new(lt_inner.clone()), - Box::new(log.clone()), - ); - manager.recover().await.unwrap(); - - // The orphaned LT blob has been cleaned up. - lt_inner.get(&physical).expect_not_found(); - // The change has been removed from the log. - let remaining = log.scan().await.unwrap(); - assert!(remaining.is_empty()); - } - - #[derive(Debug)] - struct FailOnFirstCompleteMultipartAttempt { - attempt: Mutex, - } - - impl FailOnFirstCompleteMultipartAttempt { - fn new() -> Self { - Self { - attempt: Mutex::new(0), - } - } - } - - #[async_trait::async_trait] - impl Hooks for FailOnFirstCompleteMultipartAttempt { - async fn complete_multipart( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - upload_id: &UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - let mut attempt = self.attempt.lock().await; - *attempt += 1; - if *attempt == 1 { - Err(Error::with_source( - ErrorKind::BackendFailure, - std::io::Error::new(std::io::ErrorKind::TimedOut, "simulated network error"), - )) - } else { - Ok(inner - .complete_multipart(id, upload_id, parts, access_time) - .await - .unwrap()) - } - } - } - - /// The first attempt to `complete_multipart` fails, which generates a `Change` entry. - /// The second call succeeds. - /// When it's time to clean up, nothing is deleted, as the `complete_multipart` eventually went - /// through before the cleanup deadline. - #[tokio::test] - async fn multipart_complete_succeeds_on_retry_and_leaves_state_consistent() { - let hv = InMemoryBackend::new("hv"); - let lt_inner = InMemoryBackend::new("lt"); - let log = InMemoryChangeLog::default(); - let storage = TieredStorage::new( - Box::new(hv.clone()), - Box::new(TestBackend::with_inner( - lt_inner.clone(), - FailOnFirstCompleteMultipartAttempt::new(), - )), - Box::new(log.clone()), - ); - - let id = make_id("mp-retry"); - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - let tiered_id: TieredUploadId = (&upload_id).try_into().unwrap(); - let physical = ObjectId { - context: id.context.clone(), - key: tiered_id.revision, - }; - - let payload = vec![0xABu8; 2 * 1024 * 1024]; - let etag = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload.clone()), - ) - .await - .unwrap(); - - // The first `complete_multipart` call fails. - let result = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag.clone(), - }], - Timestamp::now(), - ) - .await; - assert!(result.is_err()); - storage.join().await; - - // The second `complete_multipart` call succeeds. - let result = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await; - assert!(result.is_ok()); - storage.join().await; - - // The object is there. - let (_, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - assert_eq!(body, payload); - - // Simulate the passage of time and run recovery. - log.expire_all(); - let manager = ChangeManager::new( - Box::new(hv.clone()), - Box::new(lt_inner.clone()), - Box::new(log.clone()), - ); - manager.recover().await.unwrap(); - - // The LT blob has not been cleaned up, as the write eventually went through. - lt_inner.get(&physical).expect_object(); - // The tombstone still points to the blob. - let tombstone = hv.get(&id).expect_tombstone(); - assert_eq!(tombstone.target, physical); - // The change has been removed from the log. - let remaining = log.scan().await.unwrap(); - assert!(remaining.is_empty()); - - // The object is still there after recovery. - let (_, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - assert_eq!(body, payload); - } - - #[derive(Debug)] - struct FailOnFirstGetMetadataAttempt { - attempt: Mutex, - } - - impl FailOnFirstGetMetadataAttempt { - fn new() -> Self { - Self { - attempt: Mutex::new(0), - } - } - } - - #[async_trait::async_trait] - impl Hooks for FailOnFirstGetMetadataAttempt { - async fn get_metadata( - &self, - inner: &InMemoryBackend, - id: &ObjectId, - access_time: Timestamp, - ) -> Result { - let mut attempt = self.attempt.lock().await; - *attempt += 1; - if *attempt == 1 { - Err(Error::with_source( - ErrorKind::BackendFailure, - std::io::Error::new(std::io::ErrorKind::TimedOut, "simulated network error"), - )) - } else { - inner.get_metadata(id, access_time).await - } - } - } - - /// The first attempt to `complete_multipart` succeeds on the LT backend, but the subsequent - /// `get_metadata` call fails with a network error, causing the overall `complete_multipart` - /// to fail. The second call retries and succeeds (the LT object already exists from the first - /// attempt). - /// When it's time to clean up, nothing is deleted, as the `complete_multipart` eventually went - /// through before the cleanup deadline. - #[tokio::test] - async fn multipart_complete_succeeds_on_retry_if_get_metadata_errs_and_leaves_state_consistent() - { - let hv = InMemoryBackend::new("hv"); - let lt_inner = InMemoryBackend::new("lt"); - let log = InMemoryChangeLog::default(); - let storage = TieredStorage::new( - Box::new(hv.clone()), - Box::new(TestBackend::with_inner( - lt_inner.clone(), - FailOnFirstGetMetadataAttempt::new(), - )), - Box::new(log.clone()), - ); - - let id = make_id("mp-retry-meta"); - let upload_id = storage - .initiate_multipart(&id, &Metadata::default()) - .await - .unwrap(); - - let tiered_id: TieredUploadId = (&upload_id).try_into().unwrap(); - let physical = ObjectId { - context: id.context.clone(), - key: tiered_id.revision, - }; - - let payload = vec![0xABu8; 2 * 1024 * 1024]; - let etag = storage - .upload_part( - &id, - &upload_id, - NonZeroU32::new(1).unwrap(), - payload.len() as u64, - None, - stream::single(payload.clone()), - ) - .await - .unwrap(); - - // The first `complete_multipart` call fails (get_metadata network error), even though it - // internally creates the LT blob. - let result = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag: etag.clone(), - }], - Timestamp::now(), - ) - .await; - assert!(result.is_err()); - storage.join().await; - - // The second `complete_multipart` call succeeds. - let result = storage - .complete_multipart( - &id, - &upload_id, - vec![CompletedPart { - part_number: NonZeroU32::new(1).unwrap(), - etag, - }], - Timestamp::now(), - ) - .await; - assert!(result.is_ok()); - storage.join().await; - - // The object is there. - let (_, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - assert_eq!(body, payload); - - // Simulate the passage of time and run recovery. - log.expire_all(); - let manager = ChangeManager::new( - Box::new(hv.clone()), - Box::new(lt_inner.clone()), - Box::new(log.clone()), - ); - manager.recover().await.unwrap(); - - // The LT blob has not been cleaned up, as the write eventually went through. - lt_inner.get(&physical).expect_object(); - // The tombstone still points to the blob. - let tombstone = hv.get(&id).expect_tombstone(); - assert_eq!(tombstone.target, physical); - // The change has been removed from the log. - let remaining = log.scan().await.unwrap(); - assert!(remaining.is_empty()); - - // The object is there after recovery. - let (_, _, s) = storage - .get_object(&id, Timestamp::now(), None) - .await - .unwrap() - .unwrap(); - let body = stream::read_to_vec(s).await.unwrap(); - assert_eq!(body, payload); - } } diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 657d55b0..a3bd8f69 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -46,8 +46,6 @@ impl StdError for Panic {} pub enum ErrorKind { /// Object metadata supplied by a client is invalid. InvalidMetadata, - /// A multipart upload identifier is invalid. - InvalidUploadId, /// A client-provided request stream failed. ClientStream, /// A requested byte range cannot be resolved against the object size. @@ -108,7 +106,6 @@ impl fmt::Display for ErrorKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::InvalidMetadata => f.write_str("invalid object metadata"), - Self::InvalidUploadId => f.write_str("invalid upload id"), Self::ClientStream => f.write_str("invalid client stream"), Self::RangeNotSatisfiable { total } => { write!(f, "range not satisfiable (object size: {total} bytes)") @@ -207,7 +204,6 @@ impl Error { match self.kind { // Malformed client input at DEBUG level ErrorKind::InvalidMetadata => Level::DEBUG, - ErrorKind::InvalidUploadId => Level::DEBUG, ErrorKind::ClientStream => Level::DEBUG, ErrorKind::RangeNotSatisfiable { .. } => Level::DEBUG, ErrorKind::UploadOffsetMismatch { .. } => Level::DEBUG, @@ -343,12 +339,6 @@ impl From for Error { } } -impl From for Error { - fn from(source: objectstore_types::multipart::InvalidUploadId) -> Self { - Self::with_source(ErrorKind::InvalidUploadId, source) - } -} - /// Result type for service operations. pub type Result = std::result::Result; diff --git a/objectstore-service/src/lib.rs b/objectstore-service/src/lib.rs index 4b753ea4..06e5e8cf 100644 --- a/objectstore-service/src/lib.rs +++ b/objectstore-service/src/lib.rs @@ -10,7 +10,6 @@ pub mod encryption; pub mod error; mod gcp_auth; pub mod id; -pub mod multipart; pub mod resumable; pub mod service; pub mod stream; diff --git a/objectstore-service/src/multipart.rs b/objectstore-service/src/multipart.rs deleted file mode 100644 index b06e4814..00000000 --- a/objectstore-service/src/multipart.rs +++ /dev/null @@ -1,71 +0,0 @@ -//! Shared types for Objectstore's multipart upload protocol. - -use std::time::SystemTime; - -pub use objectstore_types::multipart::{ETag, InvalidUploadId, PartNumber, UploadId}; - -/// Description of one part in the response to -/// [`MultipartUploadBackend::list_parts`](crate::backend::common::MultipartUploadBackend::list_parts). -#[derive(Clone, Debug)] -pub struct Part { - /// 1-indexed position of this part within the upload. - pub part_number: PartNumber, - /// Identifier returned when the part was uploaded. - pub etag: ETag, - /// Server-recorded time at which the part was uploaded. - pub last_modified: SystemTime, - /// Size of the part in bytes. - pub size: u64, -} - -/// Pair of (part number, ETag) that the client provides on -/// [`MultipartUploadBackend::complete_multipart`](crate::backend::common::MultipartUploadBackend::complete_multipart) -/// to identify the parts in order. -#[derive(Clone, Debug)] -pub struct CompletedPart { - /// 1-indexed position of this part within the upload. - pub part_number: PartNumber, - /// Identifier returned when the part was uploaded. - pub etag: ETag, -} - -/// Error optionally returned by -/// [`MultipartUploadBackend::complete_multipart`](crate::backend::common::MultipartUploadBackend::complete_multipart). -#[derive(Clone, Debug)] -pub struct CompleteMultipartError { - /// Error code or identifier. - pub code: String, - /// Human-readable error description. - pub message: String, -} - -/// Response for -/// [`MultipartUploadBackend::initiate_multipart`](crate::backend::common::MultipartUploadBackend::initiate_multipart). -pub type InitiateMultipartResponse = UploadId; - -/// Response for -/// [`MultipartUploadBackend::upload_part`](crate::backend::common::MultipartUploadBackend::upload_part). -pub type UploadPartResponse = ETag; - -/// Response for -/// [`MultipartUploadBackend::list_parts`](crate::backend::common::MultipartUploadBackend::list_parts). -#[derive(Clone, Debug)] -pub struct ListPartsResponse { - /// Parts uploaded so far, in `part_number` order. - pub parts: Vec, - /// Set when the listing was truncated and more parts can be fetched - /// using [`Self::next_part_number_marker`] as the next - /// `part_number_marker`. - pub is_truncated: bool, - /// Marker to pass as the next `part_number_marker` when - /// [`Self::is_truncated`] is `true`. - pub next_part_number_marker: Option, -} - -/// Response for -/// [`MultipartUploadBackend::abort_multipart`](crate::backend::common::MultipartUploadBackend::abort_multipart). -pub type AbortMultipartResponse = (); - -/// Response for -/// [`MultipartUploadBackend::complete_multipart`](crate::backend::common::MultipartUploadBackend::complete_multipart). -pub type CompleteMultipartResponse = Option; diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index 38ac193d..c6c7ff07 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -26,10 +26,6 @@ use crate::concurrency::ConcurrencyLimiter; use crate::encryption::Cipher; use crate::error::{ErrorKind, Result, ResultExt as _}; use crate::id::{ObjectContext, ObjectId}; -use crate::multipart::{ - AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse, - ListPartsResponse, PartNumber, UploadId, UploadPartResponse, -}; use crate::resumable::Session; use crate::stream::{ClientStream, PayloadStream}; use crate::streaming::StreamExecutor; @@ -362,116 +358,6 @@ impl StorageService { self.inner.join().await; } - // --- Multipart upload operations --- - - /// Initiates a new multipart upload. - pub async fn initiate_multipart( - &self, - id: ObjectId, - metadata: Metadata, - ) -> Result { - metadata.validate().kind(ErrorKind::InvalidMetadata)?; - self.inner.as_multipart_upload_backend()?; // Fail before clone/spawn if unsupported - let inner = self.inner.clone(); - self.spawn("initiate_multipart", async move { - inner - .as_multipart_upload_backend()? - .initiate_multipart(&id, &metadata) - .await - }) - .await - } - - /// Uploads a single part. - /// - /// Note that this requires a `content_length`. - /// This grants us the broadest and most seamless compatibility when it comes to backends. - /// For example, MinIO rejects `UploadPart` requests without a `Content-Length` on plain PUT - /// requests. - /// This can be worked around by using AWS SigV4 chunked streaming requests, which we could use - /// if one day we'll have a usecase where the client doesn't know the part length upfront. - pub async fn upload_part( - &self, - id: ObjectId, - upload_id: UploadId, - part_number: PartNumber, - content_length: u64, - content_md5: Option, - body: ClientStream, - ) -> Result { - self.inner.as_multipart_upload_backend()?; // Fail before clone/spawn if unsupported - let inner = self.inner.clone(); - self.spawn("upload_part", async move { - inner - .as_multipart_upload_backend()? - .upload_part( - &id, - &upload_id, - part_number, - content_length, - content_md5.as_deref(), - body, - ) - .await - }) - .await - } - - /// Lists the parts uploaded so far. - pub async fn list_parts( - &self, - id: ObjectId, - upload_id: UploadId, - max_parts: Option, - part_number_marker: Option, - ) -> Result { - self.inner.as_multipart_upload_backend()?; // Fail before clone/spawn if unsupported - let inner = self.inner.clone(); - self.spawn("list_parts", async move { - inner - .as_multipart_upload_backend()? - .list_parts(&id, &upload_id, max_parts, part_number_marker) - .await - }) - .await - } - - /// Aborts a multipart upload. - pub async fn abort_multipart( - &self, - id: ObjectId, - upload_id: UploadId, - ) -> Result { - self.inner.as_multipart_upload_backend()?; // Fail before clone/spawn if unsupported - let inner = self.inner.clone(); - self.spawn("abort_multipart", async move { - inner - .as_multipart_upload_backend()? - .abort_multipart(&id, &upload_id) - .await - }) - .await - } - - /// Finalizes a multipart upload. - pub async fn complete_multipart( - &self, - id: ObjectId, - upload_id: UploadId, - parts: Vec, - access_time: Timestamp, - ) -> Result { - self.inner.as_multipart_upload_backend()?; // Fail before clone/spawn if unsupported - let inner = self.inner.clone(); - self.spawn("complete_multipart", async move { - inner - .as_multipart_upload_backend()? - .complete_multipart(&id, &upload_id, parts, access_time) - .await - }) - .await - } - // --- Resumable upload operations --- /// Opens a resumable upload session for an object of `upload_length` bytes. diff --git a/objectstore-test/src/server.rs b/objectstore-test/src/server.rs index 459a6a65..7ea9641d 100644 --- a/objectstore-test/src/server.rs +++ b/objectstore-test/src/server.rs @@ -15,7 +15,7 @@ use std::collections::BTreeMap; use std::net::{IpAddr, SocketAddr, TcpListener}; use objectstore_server::config::{ - AuthZVerificationKey, Config, MultipartUploadStorageConfig, StorageConfig, + AuthZVerificationKey, Config, LongTermStorageConfig, StorageConfig, }; use objectstore_server::state::Services; use objectstore_server::web::App; @@ -136,7 +136,7 @@ fn replace_fs_paths(config: &mut StorageConfig, tempdirs: &mut Vec) { tempdirs.push(dir); } StorageConfig::Tiered(c) => { - if let MultipartUploadStorageConfig::FileSystem(fs) = &mut c.long_term { + if let LongTermStorageConfig::FileSystem(fs) = &mut c.long_term { let dir = tempfile::tempdir().unwrap(); fs.path = dir.path().into(); tempdirs.push(dir); diff --git a/objectstore-types/src/lib.rs b/objectstore-types/src/lib.rs index fffe7f0d..c280d9c2 100644 --- a/objectstore-types/src/lib.rs +++ b/objectstore-types/src/lib.rs @@ -9,7 +9,6 @@ pub mod auth; pub mod duration; pub mod headers; pub mod metadata; -pub mod multipart; pub mod presign; pub mod range; pub mod resumable; diff --git a/objectstore-types/src/multipart.rs b/objectstore-types/src/multipart.rs deleted file mode 100644 index 402fe2a2..00000000 --- a/objectstore-types/src/multipart.rs +++ /dev/null @@ -1,172 +0,0 @@ -//! Types for the multipart upload protocol. -//! -//! Multipart uploads split large objects into independently uploaded parts that -//! are assembled server-side on completion. This module defines the request and -//! response types exchanged between clients and the server during each phase of -//! the protocol: initiation, part upload, part listing, and completion. -//! -//! Key types: -//! - [`UploadId`] — opaque, path-safe identifier for an in-progress upload. -//! - [`InitiateResponse`] — returned when a new upload is created. -//! - [`CompletePart`] / [`CompleteRequest`] — part manifest sent to finalize. -//! - [`PartInfo`] / [`ListPartsResponse`] — used to resume interrupted uploads. - -use std::fmt; -use std::num::NonZeroU32; -use std::ops::Deref; -use std::path::{Component, Path}; -use std::time::SystemTime; - -use serde::{Deserialize, Deserializer, Serialize}; - -/// 1-indexed position of a part within its multipart upload. -pub type PartNumber = NonZeroU32; - -/// Opaque entity tag identifying a specific version of an uploaded part. -pub type ETag = String; - -/// Identifier for an in-progress multipart upload. -/// -/// Validated on construction: non-empty and free of path-traversal components -/// (`..`, leading `/`, etc.), so it is always safe to use as a single path segment. -#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize)] -#[serde(transparent)] -pub struct UploadId(String); - -/// Error returned when an [`UploadId`] fails validation. -#[derive(Debug, thiserror::Error)] -#[error("invalid upload_id: {0}")] -pub struct InvalidUploadId(String); - -impl UploadId { - /// Creates a new `UploadId` after validating the input. - pub fn new(s: String) -> Result { - if s.is_empty() { - return Err(InvalidUploadId("must not be empty".into())); - } - for component in Path::new(&s).components() { - if !matches!(component, Component::Normal(_)) { - return Err(InvalidUploadId(s)); - } - } - Ok(Self(s)) - } - - /// Returns the upload ID as a string slice. - pub fn as_str(&self) -> &str { - &self.0 - } -} - -impl Deref for UploadId { - type Target = str; - fn deref(&self) -> &str { - &self.0 - } -} - -impl fmt::Display for UploadId { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.fmt(f) - } -} - -impl<'de> Deserialize<'de> for UploadId { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - Self::new(s).map_err(serde::de::Error::custom) - } -} - -/// Response from initiating a multipart upload. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct InitiateResponse { - /// The object key (server-generated or user-provided). - pub key: String, - /// The upload session identifier for subsequent requests. - pub upload_id: UploadId, -} - -/// Response from uploading a single part. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct UploadPartResponse { - /// Opaque identifier of the uploaded part. - pub etag: ETag, -} - -/// Information about a single uploaded part, as returned by list-parts. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct PartInfo { - /// The part number. - pub part_number: PartNumber, - /// Opaque identifier of the part. - pub etag: ETag, - /// When the part was last modified. - #[serde(with = "humantime_serde")] - pub last_modified: SystemTime, - /// Size of the part in bytes. - pub size: u64, -} - -impl From for CompletePart { - fn from(info: PartInfo) -> Self { - Self { - part_number: info.part_number, - etag: info.etag, - } - } -} - -/// Response from listing parts of a multipart upload. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ListPartsResponse { - /// Parts uploaded so far. - pub parts: Vec, - /// Whether the response was truncated. - pub is_truncated: bool, - /// Marker for the next page of results, if truncated. - #[serde(skip_serializing_if = "Option::is_none")] - pub next_part_number_marker: Option, -} - -/// A single part reference used in the complete request. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CompletePart { - /// The part number. - pub part_number: PartNumber, - /// The etag returned when this part was uploaded. - pub etag: ETag, -} - -/// Request body for completing a multipart upload. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CompleteRequest { - /// Ordered list of all parts that make up the object. - pub parts: Vec, -} - -/// Successful response from completing a multipart upload. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CompleteSuccessResponse { - /// The object key. - pub key: String, -} - -/// Detail of an error that occurred during multipart completion. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CompleteErrorDetail { - /// Error code. - pub code: String, - /// Human-readable error message. - pub message: String, -} - -/// Error response from completing a multipart upload. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CompleteErrorResponse { - /// The error detail. - pub error: CompleteErrorDetail, -} diff --git a/objectstore-types/src/time.rs b/objectstore-types/src/time.rs index c55bb965..d1c1bac0 100644 --- a/objectstore-types/src/time.rs +++ b/objectstore-types/src/time.rs @@ -2,7 +2,7 @@ //! //! [`Timestamp`] represents object creation times, expiration deadlines, and the access times //! used to check or renew them. Fractional timestamps round up to the next second. Event -//! timestamps, metrics, and multipart modification times retain their own precision. +//! timestamps and metrics retain their own precision. //! //! The default serde representation preserves the `SystemTime` metadata format. Use //! [`Timestamp::as_rfc3339`] for HTTP headers and JSON fields containing RFC3339 strings. diff --git a/stresstest/Cargo.toml b/stresstest/Cargo.toml index 73b44f10..169738b6 100644 --- a/stresstest/Cargo.toml +++ b/stresstest/Cargo.toml @@ -18,7 +18,7 @@ futures = { workspace = true } humantime = { workspace = true } humantime-serde = { workspace = true } indicatif = { workspace = true } -objectstore-client = { workspace = true, features = ["multipart"] } +objectstore-client = { workspace = true } objectstore-metrics = { path = "../objectstore-metrics" } rand = { workspace = true } rand_distr = { workspace = true } diff --git a/stresstest/README.md b/stresstest/README.md index fb981718..e67c307f 100644 --- a/stresstest/README.md +++ b/stresstest/README.md @@ -35,7 +35,6 @@ Each entry in `workloads` configures an independent, concurrent workload: | `mode` | no | `weighted` | Scheduling mode: `weighted`, `throughput`, or `batch` | | `file_sizes` | yes | — | File size distribution (see below) | | `actions` | no | 97/2/1 | Operation distribution (see below) | -| `multipart` | no | — | Multipart upload configuration (see below) | ### File sizes @@ -62,22 +61,6 @@ Controls the mix of operations. Interpretation depends on the mode: - In **batch** mode: `writes` is the number of puts per batch request; `reads` and `deletes` must be `0`. -### Multipart - -Enables multipart uploads for `actions.writes` ops of objects larger than 1 KiB. -Cannot be used in `batch` mode, but can be combined with `throughput` or `weighted`. - -| Field | Required | Description | -|---------------|----------|----------------------------------------------------------| -| `concurrency` | yes | Max concurrent part uploads per object | -| `part_size` | yes | Size of each part (min `5 MiB`); last part may be smaller | - -```yaml -multipart: - concurrency: 4 - part_size: 10 MiB -``` - ## Modes ### Weighted (default) diff --git a/stresstest/config/example.yaml b/stresstest/config/example.yaml index a3a10ec1..204e7e54 100644 --- a/stresstest/config/example.yaml +++ b/stresstest/config/example.yaml @@ -50,9 +50,6 @@ workloads: file_sizes: p50: 20 MiB p99: 100 MiB - multipart: - concurrency: 4 - part_size: 10 MiB actions: writes: 100 reads: 0 diff --git a/stresstest/src/config.rs b/stresstest/src/config.rs index 6e6a794a..3860a613 100644 --- a/stresstest/src/config.rs +++ b/stresstest/src/config.rs @@ -4,7 +4,7 @@ use std::time::Duration; use anyhow::bail; use bytesize::ByteSize; use serde::{Deserialize, Deserializer}; -use stresstest::workload::{self, WorkloadMode}; +use stresstest::workload::WorkloadMode; /// Time-to-live configuration for objects created during the stresstest. /// @@ -88,14 +88,6 @@ pub struct Workload { pub file_sizes: FileSizes, #[serde(default)] pub actions: Actions, - #[serde(default)] - pub multipart: Option, -} - -#[derive(Debug, Deserialize)] -pub struct MultipartConfig { - pub concurrency: usize, - pub part_size: ByteSize, } impl Workload { @@ -117,37 +109,8 @@ impl Workload { } } - if let Some(mp) = &self.multipart { - if matches!(self.mode, WorkloadMode::Batch) { - bail!( - "workload '{}': multipart uploads are not supported in batch mode", - self.name - ); - } - if mp.part_size.0 < 5 * 1024 * 1024 { - bail!( - "workload '{}': multipart part_size must be at least 5 MiB, got {}", - self.name, - mp.part_size - ); - } - if mp.concurrency == 0 { - bail!( - "workload '{}': multipart concurrency must be at least 1", - self.name - ); - } - } - Ok(()) } - - pub fn multipart_config(&self) -> Option { - self.multipart.as_ref().map(|mp| workload::MultipartConfig { - concurrency: mp.concurrency, - part_size: mp.part_size.0, - }) - } } fn default_concurrency() -> usize { diff --git a/stresstest/src/http.rs b/stresstest/src/http.rs index 645c1c48..b4aa4bea 100644 --- a/stresstest/src/http.rs +++ b/stresstest/src/http.rs @@ -1,13 +1,12 @@ //! Contains a remote implementation using HTTP to interact with objectstore. use anyhow::Context; -use bytes::Bytes; -use futures::{StreamExt, TryStreamExt}; +use futures::StreamExt; use objectstore_client::{Client, GetResponse, Session, TokenGenerator, Usecase}; use tokio::io::AsyncReadExt; use tokio_util::io::{ReaderStream, StreamReader}; -use crate::workload::{MultipartConfig, Payload}; +use crate::workload::Payload; /// A remote implementation using HTTP to interact with objectstore. #[derive(Debug)] @@ -45,60 +44,6 @@ impl HttpRemote { .context("error writing payload") } - pub(crate) async fn write_multipart( - &self, - usecase: &Usecase, - organization_id: u64, - payload: Payload, - config: &MultipartConfig, - ) -> anyhow::Result { - let session = self.session(usecase, organization_id); - let total_len = payload.len; - let part_size = config.part_size; - - let upload = session - .initiate_multipart_upload() - .compression(None) - .send() - .await - .context("error initiating multipart upload")?; - - let num_parts = total_len.div_ceil(part_size) as u32; - - // Lazily read chunks from the payload so that at most `concurrency` - // part buffers are in memory at any time (bounded by buffer_unordered). - let chunk_stream = futures::stream::unfold( - (payload, 1u32), - move |(mut payload, part_number)| async move { - if part_number > num_parts { - return None; - } - let chunk_len = part_size.min(payload.len) as usize; - let mut buf = vec![0u8; chunk_len]; - payload.read_exact(&mut buf).await.ok()?; - Some(((part_number, Bytes::from(buf)), (payload, part_number + 1))) - }, - ); - - let mut parts: Vec<_> = chunk_stream - .map(|(part_number, chunk)| { - let upload = &upload; - async move { upload.put(chunk, part_number, None).await } - }) - .buffer_unordered(config.concurrency) - .try_collect() - .await - .context("error uploading parts")?; - - parts.sort_by_key(|p| p.part_number); - - upload - .complete(parts) - .await - .map(|key| key.to_string()) - .context("error completing multipart upload") - } - pub(crate) async fn read( &self, usecase: &Usecase, diff --git a/stresstest/src/main.rs b/stresstest/src/main.rs index 69bc13d4..c490ce91 100644 --- a/stresstest/src/main.rs +++ b/stresstest/src/main.rs @@ -78,7 +78,6 @@ async fn main() -> anyhow::Result<()> { for w in config.workloads { w.validate()?; - let multipart = w.multipart_config(); let workload = Workload::builder(w.name) .concurrency(w.concurrency) @@ -87,7 +86,6 @@ async fn main() -> anyhow::Result<()> { .size_distribution(w.file_sizes.p50.0, w.file_sizes.p99.0) .max_size(w.file_sizes.max.map(|b| b.0)) .action_weights(w.actions.writes, w.actions.reads, w.actions.deletes) - .multipart(multipart) .build(); stresstest = stresstest.workload(workload); diff --git a/stresstest/src/stresstest.rs b/stresstest/src/stresstest.rs index de242b60..6e8af964 100644 --- a/stresstest/src/stresstest.rs +++ b/stresstest/src/stresstest.rs @@ -251,8 +251,6 @@ async fn run_workload( WorkloadMode::Throughput => 100, }; - let multipart_config = workload.multipart.clone().map(Arc::new); - let semaphore = Arc::new(Semaphore::new(concurrency)); let start = Instant::now(); let deadline = tokio::time::Instant::now() + duration; @@ -285,7 +283,6 @@ async fn run_workload( }; - let multipart_config = multipart_config.clone(); let task = async move { gauge!("stresstest.concurrency" += 1usize, workload = workload_name.clone()); let start = Instant::now(); @@ -300,12 +297,7 @@ async fn run_workload( usecase = usecase.with_expiration_policy(ExpirationPolicy::TimeToLive(ttl)); } - let use_multipart = multipart_config.is_some() && file_size > 1024; - let result = if use_multipart { - remote.write_multipart(&usecase, organization_id, payload, multipart_config.as_ref().unwrap()).await - } else { - remote.write(&usecase, organization_id, payload).await - }; + let result = remote.write(&usecase, organization_id, payload).await; match result { Ok(object_key) => { diff --git a/stresstest/src/workload.rs b/stresstest/src/workload.rs index 24be9068..53f27d17 100644 --- a/stresstest/src/workload.rs +++ b/stresstest/src/workload.rs @@ -13,15 +13,6 @@ use rand_distr::{Distribution, LogNormal, Zipf}; use serde::Deserialize; use tokio::io::{AsyncRead, ReadBuf}; -/// Configuration for multipart uploads within a workload. -#[derive(Debug, Clone)] -pub struct MultipartConfig { - /// Maximum number of concurrent part uploads. - pub concurrency: usize, - /// Size of each part in bytes (except the last part, which may be smaller). - pub part_size: u64, -} - /// Defines how the workload schedules its operations. #[derive(Debug, Default, Clone, Copy, Deserialize)] #[serde(rename_all = "snake_case")] @@ -60,8 +51,6 @@ pub struct WorkloadBuilder { write_weight: usize, read_weight: usize, delete_weight: usize, - - multipart: Option, } impl WorkloadBuilder { @@ -104,12 +93,6 @@ impl WorkloadBuilder { self } - /// Sets the multipart upload configuration. - pub fn multipart(mut self, config: Option) -> Self { - self.multipart = config; - self - } - /// Creates the workload instance. pub fn build(self) -> Workload { let rng = SmallRng::seed_from_u64(self.seed); @@ -129,7 +112,6 @@ impl WorkloadBuilder { concurrency: self.concurrency, organizations: self.organizations, mode: self.mode, - multipart: self.multipart, rng, size_distribution, @@ -162,8 +144,6 @@ pub struct Workload { pub(crate) organizations: u64, /// The target throughput for the workload, in bytes per second. Overrides concurrency. pub(crate) mode: WorkloadMode, - /// Optional multipart upload configuration. - pub(crate) multipart: Option, /// The RNG driving all our distributions. rng: SmallRng, @@ -198,8 +178,6 @@ impl Workload { write_weight: 33, read_weight: 33, delete_weight: 33, - - multipart: None, } }