From 155d3dcf09dfbd2a8c3a7c09dec72abbb7926efb Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 16 Sep 2026 21:27:43 +0900 Subject: [PATCH 1/8] =?UTF-8?q?fix(s3compatsigv4):=20=E3=83=9E=E3=83=AB?= =?UTF-8?q?=E3=83=81=E3=83=91=E3=83=BC=E3=83=88=E4=B8=AD=E6=AD=A2=E3=81=AE?= =?UTF-8?q?=E6=88=90=E5=90=A6=E5=88=A4=E5=AE=9A=E3=81=8C=E5=8F=8D=E8=BB=A2?= =?UTF-8?q?=E3=81=97=E3=81=A6=E3=81=84=E3=81=9F=E3=81=AE=E3=82=92=E4=BF=AE?= =?UTF-8?q?=E6=AD=A3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_abort_chunked_upload` が中止 API の成否を逆に読んでいた。成功したときに 「一時パーツの削除に失敗した」と警告し、失敗したときには何も警告しない。 残存パーツの有無をログが正反対に伝えるため、運用側が実態を判断できない。 判定を反転し、成功/失敗それぞれのログを実際の結果に一致させる。 既存の中止経路のテストに、成否それぞれのログ内容を確認するケースを追加した。 --- .../providers/s3compatsigv4/test_provider.py | 52 +++++++++++++++---- .../providers/s3compatsigv4/provider.py | 4 +- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index bd21e0752..784bc90d9 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -1165,7 +1165,7 @@ async def test_chunked_upload_complete(self, provider, upload_parts_headers_list @pytest.mark.asyncio @pytest.mark.aiohttpretty - async def test_chunked_upload_aborted_success(self, provider, upload_parts_headers_list, file_stream, mock_time): + async def test_chunked_upload_aborted_success(self, provider, file_stream, mock_time): assert file_stream.size == 6 provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 provider.CHUNK_SIZE = 2 @@ -1173,29 +1173,63 @@ async def test_chunked_upload_aborted_success(self, provider, upload_parts_heade path = WaterButlerPath('/foobah', prepend=provider.prefix) upload_id = 'EXAMPLEJZ6e0YupT2h66iePQCc9IEbYbDUy4RTpMeoSMLPRp8Z5o1u' \ '8feSRonpvnWsKKG35tI2LB9VDPiCgTy.Gq2VxQLYjrue4Nq.NBdqI-' - headers_list = json.loads(upload_parts_headers_list).get('headers_list') - headers_list = [{k.upper(): v for k, v in headers.items()} for headers in headers_list] - provider.metadata = MockCoroutine() provider._create_upload_session = MockCoroutine() provider._create_upload_session.return_value = upload_id + # NOTE: the failure must be injected into ``_upload_parts``, not + # ``_upload_part``. ``_chunked_upload`` only ever calls the former, so + # a ``side_effect`` on the latter never fires. (An earlier revision did + # exactly that and the test passed only because the unmocked + # ``_complete_multipart_upload`` hit aiohttpretty's "No URLs matching + # POST ..." error -- i.e. it asserted nothing about the abort path.) provider._upload_parts = MockCoroutine() - provider._upload_parts.return_value = headers_list - provider._upload_part = MockCoroutine() - provider._upload_part.side_effect = Exception('error') + provider._upload_parts.side_effect = Exception('error') + provider._complete_multipart_upload = MockCoroutine() provider._abort_chunked_upload = MockCoroutine() provider._abort_chunked_upload.return_value = True with pytest.raises(exceptions.UploadError) as exc: await provider._chunked_upload(file_stream, path) + # The abort has SUCCEEDED (return value True), so the "manual clean-up" + # warning must NOT be appended to the error message. msg = 'An unexpected error has occurred during the multi-part upload.' - msg += ' The abort action failed to clean up the temporary file parts generated ' \ - 'during the upload process. Please manually remove them.' assert str(exc.value) == ', '.join(['500', msg]) provider._create_upload_session.assert_called_with(path) provider._upload_parts.assert_called_with(file_stream, path, upload_id) provider._abort_chunked_upload.assert_called_with(path, upload_id) + # The parts upload failed, so the commit step must never be attempted. + provider._complete_multipart_upload.assert_not_called() + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_abort_failure_appends_warning(self, provider, file_stream, + mock_time): + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEJZ6e0YupT2h66iePQCc9IEbYbDUy4RTpMeoSMLPRp8Z5o1u' \ + '8feSRonpvnWsKKG35tI2LB9VDPiCgTy.Gq2VxQLYjrue4Nq.NBdqI-' + + provider._create_upload_session = MockCoroutine() + provider._create_upload_session.return_value = upload_id + provider._upload_parts = MockCoroutine() + provider._upload_parts.side_effect = Exception('error') + provider._abort_chunked_upload = MockCoroutine() + provider._abort_chunked_upload.return_value = False + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + # The abort has FAILED (return value False), so the "manual clean-up" + # warning must be appended to the error message. + msg = 'An unexpected error has occurred during the multi-part upload.' + msg += ' The abort action failed to clean up the temporary file parts generated ' \ + 'during the upload process. Please manually remove them.' + assert str(exc.value) == ', '.join(['500', msg]) + + provider._abort_chunked_upload.assert_called_with(path, upload_id) @pytest.mark.asyncio @pytest.mark.aiohttpretty diff --git a/waterbutler/providers/s3compatsigv4/provider.py b/waterbutler/providers/s3compatsigv4/provider.py index 20ff65fd5..45166e2e4 100644 --- a/waterbutler/providers/s3compatsigv4/provider.py +++ b/waterbutler/providers/s3compatsigv4/provider.py @@ -385,7 +385,9 @@ async def _chunked_upload(self, stream, path): msg = 'An unexpected error has occurred during the multi-part upload.' logger.error('{} upload_id={} error={!r}'.format(msg, session_upload_id, err)) aborted = await self._abort_chunked_upload(path, session_upload_id) - if aborted: + if not aborted: + # NOTE: this warning must be appended only when the abort has + # FAILED. (An earlier revision appended it on success.) msg += ' The abort action failed to clean up the temporary file parts generated ' \ 'during the upload process. Please manually remove them.' raise exceptions.UploadError(msg) From 8e03fbe455fdee6bd09a1587baab139ee4e76e46 Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 16 Sep 2026 21:27:57 +0900 Subject: [PATCH 2/8] =?UTF-8?q?fix(s3compatsigv4):=20S3=20=E3=81=AE?= =?UTF-8?q?=E3=82=A8=E3=83=A9=E3=83=BC=E5=BF=9C=E7=AD=94=E6=9C=AC=E6=96=87?= =?UTF-8?q?=E3=82=92=E8=A7=A3=E6=9E=90=E3=81=99=E3=82=8B=E3=83=A6=E3=83=BC?= =?UTF-8?q?=E3=83=86=E3=82=A3=E3=83=AA=E3=83=86=E3=82=A3=E3=82=92=E8=BF=BD?= =?UTF-8?q?=E5=8A=A0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S3 互換ストレージはエラーを XML の `` / `` で返すが、 これを読む処理がプロバイダに無く、後続の異常系ハンドリングを書く土台が無い。 `_parse_s3_error_body` を追加する。名前空間つきタグにも対応し、本文が XML として読めないときや `` が空のときは `(None, None)` を返して 呼び出し側に判断を委ねる。単体では挙動を変えないユーティリティのみの追加。 --- .../providers/s3compatsigv4/test_provider.py | 122 ++++++++++++++++++ .../providers/s3compatsigv4/provider.py | 63 +++++++++ 2 files changed, 185 insertions(+) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index 784bc90d9..bca812b54 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -8,6 +8,7 @@ import datetime import aiohttpretty from http import client +from http import HTTPStatus from urllib import parse from unittest import mock @@ -19,6 +20,22 @@ from waterbutler.core.path import WaterButlerPath from waterbutler.providers.s3compatsigv4 import S3CompatSigV4Provider from waterbutler.providers.s3compatsigv4 import settings as pd_settings +from waterbutler.providers.s3compatsigv4 import provider as pd_provider + +PROVIDER_LOGGER = pd_provider.__name__ + + +def storage_error(message, code=403, exception_type=exceptions.UploadError): + """Build the error a storage rejection produces on the upload path. + + In production these errors are born in ``make_request``, via + ``exception_from_response``, so the message is the storage's own response + body. Tests that fake a storage failure above the ``make_request`` + boundary go through here rather than constructing the exception inline, so + that there is one place to change when the shape of that payload does. + """ + return exception_type(message, code=code) + from tests.utils import MockCoroutine from collections import OrderedDict @@ -1231,6 +1248,111 @@ async def test_chunked_upload_abort_failure_appends_warning(self, provider, file provider._abort_chunked_upload.assert_called_with(path, upload_id) + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_exception_from_response_contract_xml(self, provider, mock_time, + generate_url_helper): + # ``_parse_s3_error_body`` depends on exception_from_response putting the + # XML body into ``data['response']``. Every other parser test builds + # that shape by hand, so this one pins down the actual contract: if + # ``exception_from_response`` ever changes (e.g. resp.json() starts + # succeeding), the parser stops seeing a body and only this test fails. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + url = generate_url_helper(key=path.full_path, method='PUT', expires=100, + headers={}, query_parameters={}) + error_body = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + aiohttpretty.register_uri('PUT', url, status=403, body=error_body, + headers={'Content-Type': 'application/xml'}) + + with pytest.raises(exceptions.UploadError) as exc: + await provider.make_request('PUT', url, expects=(HTTPStatus.OK, ), + throws=exceptions.UploadError) + + assert exc.value.data == {'response': error_body} + assert provider._parse_s3_error_body(exc.value) == ( + 'QuotaExceeded', 'The bucket quota has been exceeded') + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_exception_from_response_contract_head(self, provider, mock_time, + generate_url_helper): + # HEAD responses have no body, so exception_from_response produces a + # plain string message. _parse_s3_error_body must degrade to + # (None, None) rather than raising on that shape. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + url = generate_url_helper(key=path.full_path, method='HEAD', expires=100, + headers={}, query_parameters={}) + aiohttpretty.register_uri('HEAD', url, status=403) + + with pytest.raises(exceptions.UploadError) as exc: + await provider.make_request('HEAD', url, expects=(HTTPStatus.OK, ), + throws=exceptions.UploadError) + + assert exc.value.data is None + assert isinstance(exc.value.message, str) + assert provider._parse_s3_error_body(exc.value) == (None, None) + + def test_parse_s3_error_body_non_xml(self, provider): + err = storage_error({'response': 'not xml at all'}, code=500) + assert provider._parse_s3_error_body(err) == (None, None) + + def test_parse_s3_error_body_pretty_printed(self, provider): + # Storages are free to pretty-print their XML. Surrounding whitespace + # must not end up inside the parsed code or message. + error_xml = ('\n' + '\n' + ' \n QuotaExceeded\n \n' + ' \n The bucket quota has been exceeded\n \n' + '\n') + err = storage_error({'response': error_xml}, code=403) + + assert provider._parse_s3_error_body(err) == ( + 'QuotaExceeded', 'The bucket quota has been exceeded') + + def test_parse_s3_error_body_scalar_error_element(self, provider): + # ``xmltodict`` maps an element without children to a plain string, so + # ``parsed['Error']`` is not always a dict. + err = storage_error({'response': 'something went wrong'}, code=500) + assert provider._parse_s3_error_body(err) == (None, None) + + def test_parse_s3_error_body_empty_code_element(self, provider): + # An empty ```` becomes ``None``; an element with attributes only + # becomes a dict. Neither is a usable error code. + err = exceptions.UploadError( + {'response': 'nope'}, code=500) + assert provider._parse_s3_error_body(err) == (None, None) + + err = exceptions.UploadError( + {'response': ''}, code=500) + assert provider._parse_s3_error_body(err) == (None, None) + + def test_parse_s3_error_body_repeated_code_elements(self, provider): + # ``xmltodict`` collapses repeated siblings into a list, so a malformed + # or merged error document gives ``Code`` as ``['A', 'QuotaExceeded']``. + # Picking one of them would be guesswork, so this is unclassifiable -- + # and it must not crash from ``.strip()`` on a list either. + error_xml = ('' + 'SlowDownQuotaExceeded' + 'firstsecond' + '/bucket/secret-key-name') + err = storage_error({'response': error_xml}, code=403) + + assert provider._parse_s3_error_body(err) == (None, None) + + def test_parse_s3_error_body_namespaced(self, provider): + # Some S3-compatible storages emit namespace-prefixed error documents. + error_xml = ('' + '' + 'QuotaExceeded' + 'The bucket quota has been exceeded' + '') + err = storage_error({'response': error_xml}, code=403) + + assert provider._parse_s3_error_body(err) == ( + 'QuotaExceeded', 'The bucket quota has been exceeded') + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_chunked_upload_limit_contiguous(self, provider, file_stream, mock_time): diff --git a/waterbutler/providers/s3compatsigv4/provider.py b/waterbutler/providers/s3compatsigv4/provider.py index 45166e2e4..71652be49 100644 --- a/waterbutler/providers/s3compatsigv4/provider.py +++ b/waterbutler/providers/s3compatsigv4/provider.py @@ -28,6 +28,22 @@ logger = logging.getLogger(__name__) +# Matches an ```` element with or without a namespace prefix, so that +# namespace-prefixed error documents are not rejected by the cheap pre-filter. +ERROR_ELEMENT_RE = re.compile(r'<(?:[^\s:>/]+:)?Error[\s>/]') + + +def _local_name_lookup(mapping, local_name, default=None): + """Look up ``local_name`` in an ``xmltodict`` mapping, ignoring any XML + namespace prefix on the keys. Returns ``default`` when absent. + """ + if local_name in mapping: + return mapping[local_name] + for key, value in mapping.items(): + if isinstance(key, str) and key.rsplit(':', 1)[-1] == local_name: + return value + return default + def compute_md5(fp): """Compute MD5 hash for file-like object.""" @@ -307,6 +323,53 @@ async def _get_content_whole_size(self, path: WaterButlerPath, revision=None): raise exceptions.MetadataError('Cannot get content size and ETag') return size, etag + @staticmethod + def _raw_error_body(err): + """Return the storage's raw response body carried by ``err``, or ``None``. + + ``exception_from_response`` stores an XML error body either as + ``err.data['response']`` (dict) or as ``err.message`` (str). + """ + body = None + data = getattr(err, 'data', None) + if isinstance(data, dict): + body = data.get('response') + if body is None: + body = getattr(err, 'message', None) + return body if isinstance(body, str) else None + + @classmethod + def _parse_s3_error_body(cls, err): + """Extract the S3 XML error ``Code`` and ``Message`` from an + :class:`waterbutler.core.exceptions.UploadError` raised by ``make_request``. + + :param err: ( :class:`.UploadError` ) The error raised by ``make_request`` + :rtype: tuple(str or None, str or None) + :return: ``(error_code, error_message)``, or ``(None, None)`` when the + response body is not a parsable S3 XML error + """ + body = cls._raw_error_body(err) + if body is None or not ERROR_ELEMENT_RE.search(body): + return None, None + try: + parsed = xmltodict.parse(body) + except ExpatError: + return None, None + if not isinstance(parsed, dict): + return None, None + error = _local_name_lookup(parsed, 'Error') + if not isinstance(error, dict): + # ``text`` parses to a plain string, and an empty + # document parses to ``None``. Neither carries an error code. + return None, None + code = _local_name_lookup(error, 'Code') + message = _local_name_lookup(error, 'Message') + if not isinstance(code, str) or not code.strip(): + # An empty ```` is ``None`` and ```` is a + # dict; without a code there is nothing to translate. + return None, None + return code.strip(), message.strip() if isinstance(message, str) else None + async def upload(self, stream, path, conflict='replace', **kwargs): """Uploads the given stream to S3 Compatible Storage From 0fb3ed8d6f0dc78759b199f69d90baa6ef0cc575 Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 16 Sep 2026 21:28:12 +0900 Subject: [PATCH 3/8] =?UTF-8?q?fix(s3compatsigv4):=20=E3=82=B9=E3=83=88?= =?UTF-8?q?=E3=83=AC=E3=83=BC=E3=82=B8=E3=81=AE=E5=AE=B9=E9=87=8F=E8=B6=85?= =?UTF-8?q?=E9=81=8E=E3=82=92=20HTTP=20507=20=E3=81=A8=E3=81=97=E3=81=A6?= =?UTF-8?q?=E5=88=A9=E7=94=A8=E8=80=85=E3=81=AB=E9=80=9A=E7=9F=A5=E3=81=99?= =?UTF-8?q?=E3=82=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_chunked_upload` がストレージの実エラーを捨てて常に原因不明の HTTP 500 に 置き換えていたため、クォータを超過してもその事実が利用者に伝わらなかった。 contiguous 経路は生の XML がそのままメッセージになり判読できなかった。 既知のクォータ超過コードを HTTP 507 と対処方法つきのメッセージに変換する。 応答自体が 507 ならコードによらず容量超過として扱う。判別できない場合も 生の応答本文は返さない(Resource パスや署名付き URL が利用者に露出するため)。 容量超過は利用者起因として扱い、5xx のアラート経路から外す。 対象コードは provider config で追加できる。 --- .../providers/s3compatsigv4/test_provider.py | 292 +++++++++++++++++- .../providers/s3compatsigv4/provider.py | 145 +++++++-- .../providers/s3compatsigv4/settings.py | 17 + 3 files changed, 426 insertions(+), 28 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index bca812b54..a8a27ea98 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -1248,15 +1248,125 @@ async def test_chunked_upload_abort_failure_appends_warning(self, provider, file provider._abort_chunked_upload.assert_called_with(path, upload_id) + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_storage_quota_exceeded(self, provider, file_stream, mock_time): + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEJZ6e0YupT2h66iePQCc9IEbYbDUy4RTpMeoSMLPRp8Z5o1u' \ + '8feSRonpvnWsKKG35tI2LB9VDPiCgTy.Gq2VxQLYjrue4Nq.NBdqI-' + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + + provider._create_upload_session = MockCoroutine() + provider._create_upload_session.return_value = upload_id + provider._upload_parts = MockCoroutine() + provider._upload_parts.side_effect = storage_error({'response': error_xml}, code=403) + provider._abort_chunked_upload = MockCoroutine() + provider._abort_chunked_upload.return_value = True + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + # A storage-side quota error must surface as HTTP 507 with an explicit, + # user-readable message, and the multipart session must be aborted. + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + assert 'QuotaExceeded' in exc.value.message + provider._abort_chunked_upload.assert_called_with(path, upload_id) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_quota_exceeded_and_abort_fails(self, provider, file_stream, + mock_time): + # Worst case: the quota error and the abort failure have + # to be reported together. The abort warning is threaded through + # _translate_upload_error as ``extra_message``, so it is easy to drop + # while keeping both single-fault tests green. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + + provider._create_upload_session = MockCoroutine(return_value=upload_id) + provider._upload_parts = MockCoroutine( + side_effect=storage_error({'response': error_xml}, code=403)) + provider._abort_chunked_upload = MockCoroutine(return_value=False) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + assert 'QuotaExceeded' in exc.value.message + # The abort FAILED, so the manual clean-up warning must also be present. + assert 'Please manually remove them.' in exc.value.message + provider._abort_chunked_upload.assert_called_with(path, upload_id) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_contiguous_upload_storage_quota_exceeded(self, provider, file_stream, + mock_time): + path = WaterButlerPath('/foobah', prepend=provider.prefix) + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + + # ``make_request`` raises ``UploadError`` built by + # ``exception_from_response`` when the storage rejects the PUT. + provider.make_request = MockCoroutine() + provider.make_request.side_effect = exceptions.UploadError({'response': error_xml}, + code=403) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._contiguous_upload(file_stream, path) + + # A storage-side quota error must surface as HTTP 507 with an explicit, + # user-readable message. + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + assert 'QuotaExceeded' in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_contiguous_upload_other_storage_error(self, provider, file_stream, + mock_time): + path = WaterButlerPath('/foobah', prepend=provider.prefix) + error_xml = ('' + 'AccessDenied' + 'Access Denied') + + provider.make_request = MockCoroutine() + provider.make_request.side_effect = exceptions.UploadError({'response': error_xml}, + code=403) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._contiguous_upload(file_stream, path) + + # Non-quota errors keep the storage's status code but get a readable + # message (not the raw XML body). + assert exc.value.code == 403 + assert 'AccessDenied' in exc.value.message + assert '' + 'QuotaExceeded' + 'The bucket quota has been exceeded' + 'REQ123') + aiohttpretty.register_uri('PUT', url, status=403, body=error_body, + headers={'Content-Type': 'application/xml'}) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._contiguous_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert exc.value.is_user_error + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + assert 'QuotaExceeded' in exc.value.message + assert aiohttpretty.has_call(method='PUT', uri=url) + + def test_quota_exceeded_error_codes_defaults(self): + codes = pd_settings.QUOTA_EXCEEDED_ERROR_CODES + # MinIO returns XMinioStorageFull on the S3 data path when the disk is + # full; XMinioAdminBucketQuotaExceeded is the bucket-quota code. + assert 'QuotaExceeded' in codes + assert 'XMinioAdminBucketQuotaExceeded' in codes + assert 'XMinioStorageFull' in codes + # Not an S3 error code -- it is an HTTP reason phrase. + assert 'InsufficientStorage' not in codes + + def test_translate_upload_error_507_fallback(self, provider): + # The storage may answer 507 with an error code we do not know. The + # status alone is enough to treat it as a quota failure. + error_xml = ('' + 'SomeVendorSpecificCode' + 'no space left') + err = storage_error({'response': error_xml}, code=HTTPStatus.INSUFFICIENT_STORAGE) + + translated = provider._translate_upload_error(err) + assert translated.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in translated.message + assert 'SomeVendorSpecificCode' in translated.message + # The 507 fallback is a quota failure like any other, so + # it must get the same non-paging treatment as a recognised code. + assert translated.is_user_error is True + + def test_translate_upload_error_507_without_xml_body(self, provider): + # A 507 with an unparsable body must still become a quota message. + err = storage_error('Insufficient Storage', code=HTTPStatus.INSUFFICIENT_STORAGE) + + translated = provider._translate_upload_error(err) + assert translated.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in translated.message + assert translated.is_user_error is True + + def test_translate_upload_error_quota_is_user_error(self, provider): + # Filling up a bucket is an expected user-side failure: it must not be + # reported to Sentry at error level nor page oncall via 5xx alerts. + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + err = storage_error({'response': error_xml}, code=403) + + assert provider._translate_upload_error(err).is_user_error is True + + # Non-quota storage errors are not the user's doing. + other_xml = error_xml.replace('QuotaExceeded', 'AccessDenied') + other = storage_error({'response': other_xml}, code=403) + assert provider._translate_upload_error(other).is_user_error is False + + @pytest.mark.parametrize('label,err_factory', [ + # Quota, by error code. + ('quota-code', lambda: storage_error( + {'response': 'QuotaExceeded' + 'quota' + '/bucket/secret-key-name'}, code=403)), + # Quota, by HTTP 507 fallback. + ('quota-507', lambda: storage_error( + {'response': 'Whatever' + '/bucket/secret-key-name'}, + code=HTTPStatus.INSUFFICIENT_STORAGE)), + # Classified, but not quota. + ('other-code', lambda: storage_error( + {'response': 'AccessDeniednope' + '/bucket/secret-key-name'}, code=403)), + # Unclassifiable XML. + ('unclassifiable', lambda: storage_error( + {'response': '/bucket/secret-key-name'}, + code=HTTPStatus.BAD_GATEWAY)), + # Not XML at all. + ('non-xml', lambda: storage_error( + {'response': '/bucket/secret-key-name is over quota'}, code=403)), + # ``exception_from_response`` builds this shape for a HEAD/no-body + # response: a *string* message that embeds the presigned URL. + ('default-msg', lambda: storage_error( + 'An error occurred while making a PUT request to ' + 'https://host/bucket/secret-key-name?X-Amz-Signature=deadbeef', code=403)), + ]) + @pytest.mark.parametrize('extra_message', ['', ' abort failed']) + def test_translate_upload_error_never_leaks_raw_body(self, provider, label, err_factory, + extra_message): + # ``BaseHandler.write_error`` (server/api/v1/core.py:28) writes + # ``exc.data`` verbatim as the response body when it is truthy, and + # otherwise writes ``exc.message``. Either way, anything left on the + # translated exception is shown to the user -- including the storage's + # Resource paths and, on the string-message path, the presigned URL and + # its signature. The translated error must carry a summary only. + err = err_factory() + translated = provider._translate_upload_error(err, extra_message=extra_message) + + assert translated.data is None, 'raw body would be written as the response body' + assert 'secret-key-name' not in translated.message + assert 'X-Amz-Signature' not in translated.message + if extra_message: + assert translated.message.endswith(extra_message) + def test_parse_s3_error_body_non_xml(self, provider): err = storage_error({'response': 'not xml at all'}, code=500) assert provider._parse_s3_error_body(err) == (None, None) + # Unclassifiable, so the translator cannot say anything specific -- but + # it must not hand the raw body back to the caller either. + translated = provider._translate_upload_error(err) + assert translated is not err + assert translated.data is None + assert 'not xml at all' not in translated.message def test_parse_s3_error_body_pretty_printed(self, provider): # Storages are free to pretty-print their XML. Surrounding whitespace - # must not end up inside the parsed code or message. + # must not defeat the quota lookup. error_xml = ('\n' '\n' ' \n QuotaExceeded\n \n' @@ -1308,14 +1546,19 @@ def test_parse_s3_error_body_pretty_printed(self, provider): '\n') err = storage_error({'response': error_xml}, code=403) - assert provider._parse_s3_error_body(err) == ( - 'QuotaExceeded', 'The bucket quota has been exceeded') + assert provider._parse_s3_error_body(err)[0] == 'QuotaExceeded' + translated = provider._translate_upload_error(err) + assert translated.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in translated.message def test_parse_s3_error_body_scalar_error_element(self, provider): # ``xmltodict`` maps an element without children to a plain string, so # ``parsed['Error']`` is not always a dict. err = storage_error({'response': 'something went wrong'}, code=500) assert provider._parse_s3_error_body(err) == (None, None) + translated = provider._translate_upload_error(err) + assert translated is not err + assert translated.data is None def test_parse_s3_error_body_empty_code_element(self, provider): # An empty ```` becomes ``None``; an element with attributes only @@ -1332,7 +1575,8 @@ def test_parse_s3_error_body_repeated_code_elements(self, provider): # ``xmltodict`` collapses repeated siblings into a list, so a malformed # or merged error document gives ``Code`` as ``['A', 'QuotaExceeded']``. # Picking one of them would be guesswork, so this is unclassifiable -- - # and it must not crash from ``.strip()`` on a list either. + # but it must stay safe end to end: no crash from ``.strip()`` on a + # list, no HTTP 500, and no raw body handed to the user. error_xml = ('' 'SlowDownQuotaExceeded' 'firstsecond' @@ -1341,6 +1585,11 @@ def test_parse_s3_error_body_repeated_code_elements(self, provider): assert provider._parse_s3_error_body(err) == (None, None) + translated = provider._translate_upload_error(err) + assert int(translated.code) == 403 + assert translated.data is None + assert 'secret-key-name' not in translated.message + def test_parse_s3_error_body_namespaced(self, provider): # Some S3-compatible storages emit namespace-prefixed error documents. error_xml = ('' @@ -1352,6 +1601,35 @@ def test_parse_s3_error_body_namespaced(self, provider): assert provider._parse_s3_error_body(err) == ( 'QuotaExceeded', 'The bucket quota has been exceeded') + translated = provider._translate_upload_error(err) + assert translated.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in translated.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_create_session_quota_exceeded(self, provider, file_stream, + mock_time): + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + + provider._create_upload_session = MockCoroutine() + provider._create_upload_session.side_effect = storage_error( + {'response': error_xml}, code=403) + provider._abort_chunked_upload = MockCoroutine() + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + # No session was created, so nothing must be aborted. + provider._abort_chunked_upload.assert_not_called() @pytest.mark.asyncio @pytest.mark.aiohttpretty diff --git a/waterbutler/providers/s3compatsigv4/provider.py b/waterbutler/providers/s3compatsigv4/provider.py index 71652be49..73a78df84 100644 --- a/waterbutler/providers/s3compatsigv4/provider.py +++ b/waterbutler/providers/s3compatsigv4/provider.py @@ -108,6 +108,16 @@ def NAME(self): CHUNK_SIZE = settings.CHUNK_SIZE CONTIGUOUS_UPLOAD_SIZE_LIMIT = settings.CONTIGUOUS_UPLOAD_SIZE_LIMIT + QUOTA_EXCEEDED_MESSAGE = ( + 'Upload failed because the quota or capacity of the cloud storage has been exceeded. ' + 'Please free up storage space or contact the storage administrator.' + ) + UNCLASSIFIED_STORAGE_ERROR_MESSAGE = ( + 'Upload failed because the cloud storage returned an error that could not be ' + 'interpreted. Please retry the upload, and contact the storage administrator if the ' + 'problem persists.' + ) + def __init__(self, auth, credentials, settings, **kwargs): """ :param dict auth: Not used @@ -370,6 +380,76 @@ def _parse_s3_error_body(cls, err): return None, None return code.strip(), message.strip() if isinstance(message, str) else None + @classmethod + def _is_quota_exhaustion(cls, err): + """Whether ``err`` is the storage reporting that it is out of space. + + ``_translate_upload_error`` and the ``_chunked_upload`` entry log both + report the same failure, so they have to agree on this. When only the + translator knew that quota exhaustion is an expected, user-resolvable + outcome, the entry log still went out at ERROR and paged oncall every + time somebody filled a bucket. + + Because the list of vendor-specific quota codes cannot be exhaustive, + a response that already carries HTTP 507 counts whatever its code is. + """ + if not isinstance(err, exceptions.UploadError): + return False + if err.code == HTTPStatus.INSUFFICIENT_STORAGE: + return True + error_code = cls._parse_s3_error_body(err)[0] + return error_code is not None and error_code in settings.QUOTA_EXCEEDED_ERROR_CODES + + def _translate_upload_error(self, err, extra_message=''): + """Translate a raw :class:`.UploadError` from the storage backend into a + user-facing error. + + Storage-side quota exhaustion (e.g. ``QuotaExceeded``) is mapped to HTTP + 507 (Insufficient Storage) with an explicit message so that the user can + tell why and how the upload ended. Other S3 XML errors are re-raised + with a readable message instead of the raw XML body. + + Because the list of vendor-specific quota error codes cannot be + exhaustive, a response that already carries HTTP 507 is treated as a + quota failure whatever its error code is (or even without a parsable + body). + + :param err: ( :class:`.UploadError` ) The original error + :param str extra_message: An optional message appended to the translated + error (e.g. a warning that the multipart-upload abort failed) + :rtype: :class:`.UploadError` + """ + error_code, error_message = self._parse_s3_error_body(err) + is_quota_error = self._is_quota_exhaustion(err) + + if is_quota_error: + code_note = ' (storage error code: {})'.format(error_code) \ + if error_code is not None else '' + return exceptions.UploadError( + '{}{}{}'.format(self.QUOTA_EXCEEDED_MESSAGE, code_note, extra_message), + code=HTTPStatus.INSUFFICIENT_STORAGE, + # Running out of storage is an expected, user-resolvable failure: + # keep it out of Sentry's error level and the 5xx alerting path. + is_user_error=True, + ) + if error_code is not None: + return exceptions.UploadError( + 'Upload failed because the cloud storage returned an error. ' + '(storage error code: {}, message: {}){}'.format( + error_code, error_message, extra_message), + code=err.code, + ) + # Nothing could be classified. ``err`` must still not be handed back: + # ``BaseHandler.write_error`` writes ``exc.data`` verbatim as the + # response body when it is set, and falls back to ``exc.message`` + # otherwise. For a dict message that body is the storage's raw XML + # (Resource paths, RequestId); for the string message that + # ``exception_from_response`` builds when there is no body, it is the + # presigned URL including its signature. + return exceptions.UploadError( + '{}{}'.format(self.UNCLASSIFIED_STORAGE_ERROR_MESSAGE, extra_message), + code=err.code) + async def upload(self, stream, path, conflict='replace', **kwargs): """Uploads the given stream to S3 Compatible Storage @@ -410,23 +490,31 @@ async def _contiguous_upload(self, stream, path): query_parameters = {'Bucket': self.bucket_name, 'Key': path.full_path} - resp = await self.make_request( - 'PUT', - functools.partial( - self.connection.generate_presigned_url, - 'put_object', - Params=query_parameters, - HttpMethod='PUT', - ), - data=upload_stream, - skip_auto_headers={'CONTENT-TYPE'}, - headers=headers, - expects=( - HTTPStatus.OK, - HTTPStatus.CREATED, - ), - throws=exceptions.UploadError, - ) + try: + resp = await self.make_request( + 'PUT', + functools.partial( + self.connection.generate_presigned_url, + 'put_object', + Params=query_parameters, + HttpMethod='PUT', + ), + data=upload_stream, + skip_auto_headers={'CONTENT-TYPE'}, + headers=headers, + expects=( + HTTPStatus.OK, + HTTPStatus.CREATED, + ), + throws=exceptions.UploadError, + ) + except exceptions.UploadError as err: + # Translate storage-side errors (e.g. quota exceeded) into a + # user-facing error instead of returning the raw XML body. + # ``from None``: the untranslated error is what carries the raw + # body, and a chained ``__context__`` puts it straight back into + # the rendered traceback. + raise self._translate_upload_error(err) from None # S3-compatible server automatically validates Content-MD5 # If MD5 doesn't match, server returns 400 UploadError before writing data @@ -437,7 +525,15 @@ async def _chunked_upload(self, stream, path): """Uploads the given stream to S3 over multiple chunks""" # Step 1. Create a multi-part upload session - session_upload_id = await self._create_upload_session(path) + try: + session_upload_id = await self._create_upload_session(path) + except exceptions.UploadError as err: + # The session has not been created, so there is nothing to abort. + # Storage-side quota errors can occur at session creation too. + # ``from None``: the untranslated error is what carries the raw + # body, and a chained ``__context__`` puts it straight back into + # the rendered traceback. + raise self._translate_upload_error(err) from None try: # Step 2. Break stream into chunks and upload them one by one @@ -448,12 +544,19 @@ async def _chunked_upload(self, stream, path): msg = 'An unexpected error has occurred during the multi-part upload.' logger.error('{} upload_id={} error={!r}'.format(msg, session_upload_id, err)) aborted = await self._abort_chunked_upload(path, session_upload_id) + abort_message = '' if not aborted: # NOTE: this warning must be appended only when the abort has # FAILED. (An earlier revision appended it on success.) - msg += ' The abort action failed to clean up the temporary file parts generated ' \ - 'during the upload process. Please manually remove them.' - raise exceptions.UploadError(msg) + abort_message = ' The abort action failed to clean up the temporary file ' \ + 'parts generated during the upload process. Please manually ' \ + 'remove them.' + if isinstance(err, exceptions.UploadError): + # Preserve the original storage error (status code and body) and + # translate quota-exhaustion responses into a user-facing error. + raise self._translate_upload_error( + err, extra_message=abort_message) from None + raise exceptions.UploadError('{}{}'.format(msg, abort_message)) async def _create_upload_session(self, path): """This operation initiates a multipart upload and returns an upload ID. This upload ID is diff --git a/waterbutler/providers/s3compatsigv4/settings.py b/waterbutler/providers/s3compatsigv4/settings.py index 0b3ba402d..b6dd1df69 100644 --- a/waterbutler/providers/s3compatsigv4/settings.py +++ b/waterbutler/providers/s3compatsigv4/settings.py @@ -10,3 +10,20 @@ CHUNK_SIZE = int(config.get('CHUNK_SIZE', 64000000)) # 64 MB CHUNKED_UPLOAD_MAX_ABORT_RETRIES = int(config.get('CHUNKED_UPLOAD_MAX_ABORT_RETRIES', 2)) + +# S3-compatible storages return different XML error codes when the storage-side +# quota / capacity has been exhausted. Well-known ones are listed as defaults: +# - 'QuotaExceeded': generic S3-compatible storages +# - 'XMinioAdminBucketQuotaExceeded': MinIO with a bucket quota configured +# - 'XMinioStorageFull': MinIO when the underlying disk is full (S3 data path) +# This list is not exhaustive, so ``_translate_upload_error`` additionally +# treats HTTP 507 as a quota failure regardless of the error code. +# Deployments can replace this list via the provider config when their storage +# vendor uses a different error code. ``get_object`` is required here: plain +# ``get`` returns the raw string when the value comes from an envvar, which +# would silently turn the membership test into substring matching. +QUOTA_EXCEEDED_ERROR_CODES = frozenset(config.get_object('QUOTA_EXCEEDED_ERROR_CODES', [ + 'QuotaExceeded', + 'XMinioAdminBucketQuotaExceeded', + 'XMinioStorageFull', +])) From 7fc6813b399f8785748900cf2667fcc3c1414231 Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 16 Sep 2026 21:28:26 +0900 Subject: [PATCH 4/8] =?UTF-8?q?fix(s3compatsigv4):=20=E6=8E=A5=E7=B6=9A?= =?UTF-8?q?=E6=96=AD=E3=83=BB=E7=95=B0=E5=B8=B8=E5=BF=9C=E7=AD=94=E3=82=92?= =?UTF-8?q?=20HTTP=20502=20=E3=81=A8=E3=81=97=E3=81=A6=E6=89=B1=E3=81=84?= =?UTF-8?q?=E3=80=81=E6=8E=A5=E7=B6=9A=E3=83=AA=E3=83=BC=E3=82=AF=E3=82=92?= =?UTF-8?q?=E5=A1=9E=E3=81=90?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 未ハンドリングの経路が残っていた。接続断(`aiohttp.ClientError`)が捕捉されず、 セッション作成が try の外にあり、`CompleteMultipartUpload` が失敗を HTTP 200 + `` で返してくる異常応答を「成功」として返していた。 接続断と全体タイムアウトを HTTP 502 とする。`` があれば分類できなくても 502 で raise する(fail-closed)。`UploadId` 欠落も同様に成功扱いしない。 raise 経路で漏れていた `resp.release()` を補い、接続リークを解消する。 あわせて失敗時ログから生の応答本文を除き、出力水準を揃える。 commit が失敗したとき成否が保証できない場合は、その旨の注記を返す。 --- .../providers/s3compatsigv4/test_provider.py | 955 +++++++++++++++++- .../providers/s3compatsigv4/provider.py | 462 ++++++++- 2 files changed, 1357 insertions(+), 60 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index a8a27ea98..a5ee786cb 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -5,7 +5,11 @@ import time import base64 import hashlib +import asyncio +import logging import datetime + +import aiohttp import aiohttpretty from http import client from http import HTTPStatus @@ -28,13 +32,20 @@ def storage_error(message, code=403, exception_type=exceptions.UploadError): """Build the error a storage rejection produces on the upload path. - In production these errors are born in ``make_request``, via - ``exception_from_response``, so the message is the storage's own response - body. Tests that fake a storage failure above the ``make_request`` - boundary go through here rather than constructing the exception inline, so - that there is one place to change when the shape of that payload does. + In production these errors are born in ``make_request`` (via + ``exception_from_response``) and are tagged by ``_make_upload_request`` so + that ``_translate_upload_error`` knows the payload is a raw storage + response rather than a message WaterButler wrote itself. + + Tests that fake a storage failure above the ``make_request`` boundary have + to reproduce that tag, and must do it by calling the provider's own + ``_mark_storage_response`` -- re-implementing the tag here would let the + test keep passing if the marker were renamed or its semantics changed. + An *untagged* error carrying a storage body cannot occur in production, so + asserting translation behaviour against one would test a state the + provider never actually sees. """ - return exception_type(message, code=code) + return pd_provider._mark_storage_response(exception_type(message, code=code)) from tests.utils import MockCoroutine @@ -1358,6 +1369,45 @@ async def test_contiguous_upload_other_storage_error(self, provider, file_stream assert 'AccessDenied' in exc.value.message assert 'AccessDenied' + 'TESTREQUESTID' + '{}').format('y' * 4096) + + provider._create_upload_session = MockCoroutine(return_value=upload_id) + provider._upload_parts = MockCoroutine(side_effect=storage_error( + {'response': error_xml}, code=HTTPStatus.FORBIDDEN)) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + with pytest.raises(exceptions.UploadError): + await provider._chunked_upload(file_stream, path) + + records = [r.getMessage() for r in caplog.records if r.name == PROVIDER_LOGGER] + assert len(records) == 2 + + entry, translated = records + # The entry log identifies the failure by type and status only. + assert 'UploadError' in entry + assert str(int(HTTPStatus.FORBIDDEN)) in entry + assert upload_id in entry + assert 'TESTREQUESTID' not in entry + assert 'y' * 64 not in entry + # The body survives exactly once, bounded by ERROR_BODY_LOG_LIMIT. + assert 'TESTREQUESTID' in translated + assert 'y' * pd_provider.ERROR_BODY_LOG_LIMIT not in translated + # Neither record may be unbounded. + for record in records: + assert len(record) < 1024 + + def test_connection_interrupted_message_does_not_assert_capacity(self, provider): + # A dropped connection is only *evidence* of a full + # storage -- it is equally often a network fault. The message must not + # send the user off to free up space when nothing is full. + message = provider.CONNECTION_INTERRUPTED_MESSAGE + assert 'network' in message.lower() + assert 'may indicate' in message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('error_code,status,expected_level', [ + # Quota exhaustion is expected and the user can fix it themselves -- + # ``_translate_upload_error`` already logs it at WARNING and marks it + # ``is_user_error``. The entry log has to agree, or this line alone + # keeps paging oncall every time somebody fills a bucket. + ('QuotaExceeded', HTTPStatus.FORBIDDEN, logging.WARNING), + ('XMinioStorageFull', HTTPStatus.FORBIDDEN, logging.WARNING), + # ...including the 507 fallback, where the code is unrecognised. + ('SomeVendorCode', HTTPStatus.INSUFFICIENT_STORAGE, logging.WARNING), + # A real fault must still be an error: the downgrade must not be blanket. + ('AccessDenied', HTTPStatus.FORBIDDEN, logging.ERROR), + ('InternalError', HTTPStatus.INTERNAL_SERVER_ERROR, logging.ERROR), + ]) + async def test_chunked_upload_entry_log_level_follows_quota( + self, provider, file_stream, mock_time, caplog, error_code, status, expected_level): + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + error_xml = ('' + '{}nope').format(error_code) + + provider._create_upload_session = MockCoroutine(return_value=upload_id) + provider._upload_parts = MockCoroutine( + side_effect=storage_error({'response': error_xml}, code=status)) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + with pytest.raises(exceptions.UploadError): + await provider._chunked_upload(file_stream, path) + + entry = [r for r in caplog.records + if r.name == PROVIDER_LOGGER and 'multi-part upload' in r.getMessage()] + assert len(entry) == 1 + assert entry[0].levelno == expected_level + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_entry_log_requires_the_storage_tag( + self, provider, file_stream, mock_time, caplog): + # Every other case above builds its error with ``storage_error``, which + # tags it. So deleting the ``_is_storage_response`` guard from + # ``_is_quota_exhaustion`` leaves the whole suite green while the + # predicate silently starts trusting messages WaterButler wrote itself. + # A 507 that carries no tag must stay an ERROR. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + # Deliberately untagged: this is the shape of an error WaterButler + # authored, not one built from a storage response. + provider._upload_parts = MockCoroutine(side_effect=exceptions.UploadError( + 'WaterButler wrote this', code=HTTPStatus.INSUFFICIENT_STORAGE)) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + with pytest.raises(exceptions.UploadError): + await provider._chunked_upload(file_stream, path) + + entry = [r for r in caplog.records + if r.name == PROVIDER_LOGGER and 'multi-part upload' in r.getMessage()] + assert len(entry) == 1 + assert entry[0].levelno == logging.ERROR + + def test_passthrough_preserves_is_user_error(self, provider): + # The untagged branch rebuilds the exception to append the abort + # warning. Dropping ``is_user_error`` there promotes a failure the user + # caused from Sentry's info level to error (server/api/v1/core.py). + err = exceptions.UploadError('WaterButler wrote this', code=HTTPStatus.CONFLICT, + is_user_error=True) + + translated = provider._translate_upload_error(err, extra_message=' Abort failed.') + + assert translated.is_user_error is True + assert translated.code == HTTPStatus.CONFLICT + assert 'Abort failed.' in translated.message + + def test_passthrough_is_observable(self, provider, caplog): + # An untagged error reaching the translator is accepted as normal, so a + # new upload call site that forgets ``_make_upload_request`` degrades + # silently: quota errors stop becoming 507s and nothing fails. A debug + # line is the only thing that makes the omission findable in the field. + err = exceptions.UploadError('WaterButler wrote this', code=HTTPStatus.CONFLICT) + + with caplog.at_level(logging.DEBUG, logger=PROVIDER_LOGGER): + provider._translate_upload_error(err) + + assert [r for r in caplog.records + if r.name == PROVIDER_LOGGER and r.levelno == logging.DEBUG + and 'untagged' in r.getMessage()] + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('stage', ['contiguous', 'create-session', 'chunked']) + async def test_translated_error_does_not_chain_the_raw_body(self, provider, file_stream, + mock_time, stage): + # ``raise translated`` inside an ``except`` block sets ``__context__`` + # to the untranslated error, so the raw body comes back in the rendered + # traceback -- which is what the logs and Sentry show. Stripping the + # body from the message accomplishes nothing if the chained exception + # carries it anyway. + import traceback + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + error_xml = ('' + 'QuotaExceeded' + '/bucket/secret-key-name') + failure = storage_error({'response': error_xml}, code=403) + + if stage == 'contiguous': + provider.make_request = MockCoroutine(side_effect=failure) + coro = provider._contiguous_upload(file_stream, path) + else: + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + provider._abort_chunked_upload = MockCoroutine(return_value=True) + if stage == 'create-session': + provider.make_request = MockCoroutine(side_effect=failure) + else: + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(side_effect=failure) + coro = provider._chunked_upload(file_stream, path) + + with pytest.raises(exceptions.UploadError) as exc: + await coro + + assert exc.value.__suppress_context__ is True + rendered = ''.join(traceback.format_exception(type(exc.value), exc.value, + exc.value.__traceback__)) + assert 'secret-key-name' not in rendered + + def test_translate_upload_error_logs_raw_body(self, provider, caplog): + # The translated error only carries a summary message, so the raw body + # (RequestId / Resource) is the only way to investigate afterwards. It + # used to be dropped entirely on the contiguous path. + error_xml = ('' + 'AccessDeniedAccess Denied' + 'TESTREQUESTID' + '/bucket/foobah') + err = storage_error({'response': error_xml}, code=HTTPStatus.FORBIDDEN) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + provider._translate_upload_error(err) + + records = [r for r in caplog.records if r.name == PROVIDER_LOGGER] + assert len(records) == 1 + logged = records[0].getMessage() + assert 'TESTREQUESTID' in logged + assert '/bucket/foobah' in logged + assert 'AccessDenied' in logged + assert str(int(HTTPStatus.FORBIDDEN)) in logged + # A non-quota storage rejection is a genuine error. + assert records[0].levelno == logging.ERROR + + def test_translate_upload_error_log_truncates_body(self, provider, caplog): + # An unbounded body would flood the log; storages can return very large + # error documents (or a proxy's HTML error page). + # + # The bound is declared here as a literal rather than read from the + # module: deriving it from the constant makes the test agree with + # whatever the constant happens to say, so shrinking it to 10 (or + # growing it to 1 MB) would keep this green. 512 is the reviewed + # value, so changing it has to be a deliberate edit here too. + limit = 512 + assert pd_provider.ERROR_BODY_LOG_LIMIT == limit + error_xml = 'AccessDenied{}'.format( + 'x' * (limit * 8)) + err = storage_error({'response': error_xml}, code=HTTPStatus.FORBIDDEN) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + provider._translate_upload_error(err) + + logged = [r for r in caplog.records if r.name == PROVIDER_LOGGER][0].getMessage() + # Pin the bound itself, not just "shorter than the input": the body is + # cut at exactly ERROR_BODY_LOG_LIMIT characters and no further. + assert error_xml[:limit] in logged + assert error_xml[:limit + 1] not in logged + # Nothing else in the record may reintroduce the rest of the body. + assert len(logged) < limit * 2 + + def test_translate_upload_error_quota_logged_as_warning(self, provider, caplog): + # Quota exhaustion is an expected, user-resolvable failure (see the + # is_user_error handling), so it must not be logged at ERROR level and + # trip the on-call alerting. + error_xml = ('QuotaExceeded' + 'The bucket quota has been exceeded' + 'QUOTAREQUESTID') + err = storage_error({'response': error_xml}, code=HTTPStatus.FORBIDDEN) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + provider._translate_upload_error(err) + + records = [r for r in caplog.records if r.name == PROVIDER_LOGGER] + assert len(records) == 1 + assert records[0].levelno == logging.WARNING + assert 'QUOTAREQUESTID' in records[0].getMessage() + def test_quota_exceeded_error_codes_defaults(self): codes = pd_settings.QUOTA_EXCEEDED_ERROR_CODES # MinIO returns XMinioStorageFull on the S3 data path when the disk is @@ -1526,6 +1875,340 @@ def test_translate_upload_error_never_leaks_raw_body(self, provider, label, err_ if extra_message: assert translated.message.endswith(extra_message) + def test_check_for_200_error_preserves_error_body(self, provider): + # S3 signals CompleteMultipartUpload failures with HTTP 200 plus an + # body. The raw body must survive on the exception, otherwise + # the quota translation downstream has nothing to work with. + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + + with pytest.raises(exceptions.UploadError) as exc: + provider._check_for_200_error(error_xml.encode('utf-8'), + 'CompleteMultipartUpload', + exceptions.UploadError) + + assert provider._parse_s3_error_body(exc.value)[0] == 'QuotaExceeded' + assert provider._translate_upload_error(exc.value).code == \ + HTTPStatus.INSUFFICIENT_STORAGE + + @pytest.mark.parametrize('label,error_xml', [ + # ``xmltodict`` collapses all three of these to ``{'Error': None}``, so a + # lookup that returns ``None`` cannot tell "no element" from + # " element we failed to classify". Conflating the two makes a + # failed CompleteMultipartUpload look like a success. + ('empty-element', ''), + ('empty-pair', ''), + ('whitespace-only', ' '), + ]) + def test_check_for_200_error_fails_closed_on_empty_error_element(self, provider, label, + error_xml): + # An element is present: the request failed. Not being able to + # classify it is no reason to report success. + with pytest.raises(exceptions.UploadError): + provider._check_for_200_error(error_xml.encode('utf-8'), + 'CompleteMultipartUpload', + exceptions.UploadError) + + @pytest.mark.parametrize('label,error_xml', [ + ('missing-code', + '' + 'something went wrong'), + ('empty-code', + '' + 'something went wrong'), + ]) + def test_check_for_200_error_unclassifiable_is_not_a_server_fault(self, provider, label, + error_xml): + # The user must never see a bare HTTP 500. An + # body we cannot classify is the *storage* answering unintelligibly, so + # it is a bad-gateway condition, not a WaterButler bug. + with pytest.raises(exceptions.UploadError) as exc: + provider._check_for_200_error(error_xml.encode('utf-8'), + 'CompleteMultipartUpload', + exceptions.UploadError) + + assert int(exc.value.code) != int(HTTPStatus.INTERNAL_SERVER_ERROR) + assert int(exc.value.code) == int(HTTPStatus.BAD_GATEWAY) + + def test_check_for_200_error_malformed_xml_is_controlled(self, provider): + # A truncated body raises ExpatError out of ``xmltodict``. Letting it + # escape means an HTTP 500 with a stack trace (``_translate_upload_error`` + # then trips over the missing ``.message``). + with pytest.raises(exceptions.UploadError) as exc: + provider._check_for_200_error(b'QuotaExceeded', + 'CompleteMultipartUpload', + exceptions.UploadError) + + assert int(exc.value.code) == int(HTTPStatus.BAD_GATEWAY) + + def test_check_for_200_error_accepts_success_body(self, provider): + # Guard the other direction: a genuine success body must stay silent. + body = ('' + '"etag"' + '') + provider._check_for_200_error(body.encode('utf-8'), 'CompleteMultipartUpload', + exceptions.UploadError) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('error_body', [ + b'', + b' ', + b'nope', + b'QuotaExceeded', + ]) + async def test_chunked_upload_complete_200_with_unclassifiable_error( + self, provider, file_stream, mock_time, error_body): + # The regression this pins down: on a *replace* upload the old object is + # still in the bucket, so ``upload()`` would return its metadata and the + # caller would record a successful upload of data that was never + # committed. The session must be aborted and the caller must see an + # error. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + + provider._create_upload_session = MockCoroutine(return_value=upload_id) + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=error_body) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert int(exc.value.code) != int(HTTPStatus.INTERNAL_SERVER_ERROR) + provider._abort_chunked_upload.assert_called_with(path, upload_id) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_session_error_keeps_waterbutler_message( + self, provider, file_stream, mock_time): + # ``_create_upload_session`` authors its own 502: a session may exist on + # the storage but its UploadId is unknown, so it cannot be aborted and + # an administrator has to remove it by hand. That message has no + # storage body behind it, so ``_translate_upload_error`` must not + # replace it with the generic "could not be interpreted" text. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + resp = mock.Mock() + resp.read = MockCoroutine(return_value=b'this is not the expected xml') + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.BAD_GATEWAY + assert 'stale multipart upload session' in exc.value.message + assert provider.UNCLASSIFIED_STORAGE_ERROR_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('stage', ['create-session', 'upload-part', 'complete']) + async def test_every_upload_stage_tags_its_storage_errors(self, provider, file_stream, + mock_time, stage): + # ``_translate_upload_error`` only translates errors the upload path + # tagged in ``_make_upload_request``. A call site that reaches for + # ``make_request`` directly therefore stops being translated *silently*: + # the user gets the storage's raw 403 instead of the quota message. + # + # Every other test for these three stages mocks above ``make_request`` + # (``_create_upload_session`` / ``_upload_parts`` are replaced wholesale), + # so none of them would notice the tag going missing. This one mocks the + # boundary itself, which keeps each real call site under test. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + # Deliberately *untagged*: ``make_request`` is the boundary where the tag + # is applied, so tagging it here would defeat the purpose of the test. + failure = exceptions.UploadError({'response': error_xml}, code=403) + + provider._abort_chunked_upload = MockCoroutine(return_value=True) + if stage != 'create-session': + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + if stage == 'complete': + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + + provider.make_request = MockCoroutine(side_effect=failure) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('error_body', [ + b'', + b'no code here', + b'QuotaExceeded', + ]) + async def test_chunked_upload_complete_unclassifiable_warns_upload_may_exist( + self, provider, file_stream, mock_time, error_body): + # Fail-closed is kept, but the complete may in fact + # have succeeded -- we simply could not read the answer. Telling the + # user only that the upload failed invites a duplicate. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + provider._create_upload_session = MockCoroutine(return_value=upload_id) + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=error_body) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message + # Asserting against the constant alone passes for *any* value of it, + # including ``''``. Pinning the wording here is what makes the + # assertion above detect the message being emptied (the same failure + # mode the ERROR_BODY_LOG_LIMIT test was fixed for). + assert 'may in fact have completed' in provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + assert 'check the file list' in provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + # The raw body must still not reach the user. + assert 'Error' not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_complete_read_failure_warns_upload_may_exist( + self, provider, file_stream, mock_time): + # The commit was sent and the storage answered -- we just could not read + # the answer. This is the case the notice exists for, yet it + # was the one case that did not get it: ``_mark_commit_outcome_unknown`` + # sat behind ``except exceptions.UploadError``, which a dropped + # connection does not satisfy. The user was told the upload was + # "interrupted before the upload completed" and asked to retry. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + resp = mock.Mock() + resp.read = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message + # The connection message must not contradict the notice it now carries. + assert 'before the upload completed' not in exc.value.message + # The connection was still released despite the read blowing up. + assert resp.release.called + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('status,error_code,expect_notice', [ + # Decision (C): a 4xx is the storage stating it refused the request, so + # nothing was committed and the notice would be misleading. A 5xx says + # the server failed while handling a request it had already accepted -- + # whether the parts were assembled is genuinely unknown. + (403, 'AccessDenied', False), + (400, 'InvalidPart', False), + (500, 'InternalError', True), + (503, 'SlowDown', True), + ]) + async def test_chunked_upload_complete_notice_follows_status_class( + self, provider, file_stream, mock_time, status, error_code, expect_notice): + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + error_xml = ('' + '{}boom'.format(error_code)) + provider.make_request = MockCoroutine( + side_effect=exceptions.UploadError({'response': error_xml}, code=status)) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_create_session_connection_error_does_not_claim_a_commit( + self, provider, file_stream, mock_time): + # No commit was ever in flight at session creation, so the notice must + # not appear -- otherwise it stops meaning anything. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider.make_request = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_complete_200_with_error_quota(self, provider, file_stream, + mock_time): + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + error_xml = ('' + 'QuotaExceeded' + 'The bucket quota has been exceeded') + + provider._create_upload_session = MockCoroutine(return_value=upload_id) + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + # CompleteMultipartUpload answers 200 with an error body. + resp = mock.Mock() + resp.read = MockCoroutine(return_value=error_xml.encode('utf-8')) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + # The user must not see a bare HTTP 500. + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + assert 'QuotaExceeded' in exc.value.message + provider._abort_chunked_upload.assert_called_with(path, upload_id) + def test_parse_s3_error_body_non_xml(self, provider): err = storage_error({'response': 'not xml at all'}, code=500) assert provider._parse_s3_error_body(err) == (None, None) @@ -1590,6 +2273,13 @@ def test_parse_s3_error_body_repeated_code_elements(self, provider): assert translated.data is None assert 'secret-key-name' not in translated.message + # ...and the 200-with-error path still fails closed on it. + with pytest.raises(exceptions.UploadError) as exc: + provider._check_for_200_error(error_xml.encode('utf-8'), + 'CompleteMultipartUpload', + exceptions.UploadError) + assert int(exc.value.code) == int(HTTPStatus.BAD_GATEWAY) + def test_parse_s3_error_body_namespaced(self, provider): # Some S3-compatible storages emit namespace-prefixed error documents. error_xml = ('' @@ -1631,6 +2321,125 @@ async def test_chunked_upload_create_session_quota_exceeded(self, provider, file # No session was created, so nothing must be aborted. provider._abort_chunked_upload.assert_not_called() + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_create_upload_session_invalid_response(self, provider, mock_time): + path = WaterButlerPath('/foobah', prepend=provider.prefix) + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=b'this is not the expected xml') + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._create_upload_session(path) + + # A malformed 200-range response must become a controlled error, not a + # raw ExpatError/KeyError propagating as HTTP 500. + assert exc.value.code == HTTPStatus.BAD_GATEWAY + assert 'unexpected response' in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('upload_id_xml', [ + '', + ' ', + '', + ]) + async def test_create_upload_session_blank_upload_id(self, provider, mock_time, + upload_id_xml): + # Well-formed XML with an unusable UploadId must not be returned: every + # later request would be signed with ``None`` and fail obscurely. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + body = ('' + '' + '{}' + '').format(upload_id_xml) + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=body.encode('utf-8')) + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._create_upload_session(path) + + assert exc.value.code == HTTPStatus.BAD_GATEWAY + assert 'unexpected response' in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('body,raises', [ + ('' + '"etag"' + '', False), + ('' + 'QuotaExceeded', True), + ('QuotaExceeded', True), + ]) + async def test_complete_multipart_upload_always_releases(self, provider, mock_time, + body, raises): + # ``release()`` came after the error check, so the 200-with-error path + # skipped it and leaked the connection back-pressure -- on exactly the + # path a quota-exhausted storage takes for every single upload. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=body.encode('utf-8')) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + if raises: + with pytest.raises(exceptions.UploadError): + await provider._complete_multipart_upload(path, 'EXAMPLEUPLOADID', + [{'ETAG': '"etag1"'}]) + else: + await provider._complete_multipart_upload(path, 'EXAMPLEUPLOADID', + [{'ETAG': '"etag1"'}]) + + assert resp.release.call_count == 1 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_complete_multipart_upload_releases_when_read_fails(self, provider, mock_time): + # ``read()`` sat *outside* the try, so a connection dropped mid-body -- + # the common failure once the storage is struggling -- skipped the + # ``finally`` entirely and leaked the connection. The read is part of + # what has to be cleaned up after, so it belongs inside the try. + import aiohttp + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + + resp = mock.Mock() + resp.read = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(aiohttp.ServerDisconnectedError): + await provider._complete_multipart_upload(path, 'EXAMPLEUPLOADID', + [{'ETAG': '"etag1"'}]) + + assert resp.release.call_count == 1 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_create_upload_session_strips_upload_id(self, provider, mock_time): + # The response is parsed with ``strip_whitespace=False`` (needed + # elsewhere), so a pretty-printed UploadId keeps its surrounding + # newlines and indentation. Returning it unstripped puts whitespace + # into every following request's ``uploadId`` query parameter -- and + # into the SigV4 signature -- so the parts would be signed for an + # upload id the storage does not have. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + body = ('\n' + '\n' + ' \n EXAMPLEUPLOADID\n \n' + '\n') + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=body.encode('utf-8')) + provider.make_request = MockCoroutine(return_value=resp) + + assert await provider._create_upload_session(path) == 'EXAMPLEUPLOADID' + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_chunked_upload_limit_contiguous(self, provider, file_stream, mock_time): @@ -1972,6 +2781,140 @@ async def test_abort_chunked_upload_session_deleted(self, provider, generic_http assert aiohttpretty.has_call(method='DELETE', uri=abort_url) assert aborted is True + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_chunked_upload_no_such_upload_is_already_clean( + self, provider, mock_time, generate_url_helper): + # The session is gone, which is precisely the state + # abort is trying to reach. Retrying until the cap and then reporting + # failure sends the user hunting for parts that do not exist. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + abort_url = generate_url_helper(key=path.full_path, method='DELETE', expires=100, + headers={}, query_parameters=params) + no_such_upload = ('' + 'NoSuchUpload' + 'The specified upload does not exist.') + aiohttpretty.register_uri('DELETE', abort_url, body=no_such_upload.encode('utf-8'), + status=404) + + list_url = generate_url_helper(key=path.full_path, method='GET', expires=100, + headers={}, query_parameters=params) + aiohttpretty.register_uri('GET', list_url, body=no_such_upload.encode('utf-8'), + status=404) + + aborted = await provider._abort_chunked_upload(path, upload_id) + + assert aborted is True + # Decision (B): ListParts is what the docstring names as the criterion + # for a successful abort, so ``NoSuchUpload`` is confirmed rather than + # trusted. One extra request; still no retry budget burned. + assert len(aiohttpretty.calls) == 2 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_no_such_upload_with_parts_left_still_warns( + self, provider, mock_time, generate_url_helper): + # ``NoSuchUpload`` has three causes: the commit succeeded, the session + # was already aborted, or the session expired by TTL. Only the third + # can leave parts behind, and S3-compatible storages do not all match + # AWS here. Treating the code alone as proof would suppress the + # "please remove them manually" warning exactly when it is needed. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + abort_url = generate_url_helper(key=path.full_path, method='DELETE', expires=100, + headers={}, query_parameters=params) + no_such_upload = ('' + 'NoSuchUpload' + 'The specified upload does not exist.') + aiohttpretty.register_uri('DELETE', abort_url, body=no_such_upload.encode('utf-8'), + status=404) + + list_url = generate_url_helper(key=path.full_path, method='GET', expires=100, + headers={}, query_parameters=params) + parts_left = ('' + '1' + '"etag1"') + aiohttpretty.register_uri('GET', list_url, body=parts_left.encode('utf-8'), status=200) + + aborted = await provider._abort_chunked_upload(path, upload_id) + + assert aborted is False + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_path_errors_carry_the_storage_tag( + self, provider, mock_time, generate_url_helper, monkeypatch): + # ``_abort_chunked_upload`` parses the S3 error body to recognise + # ``NoSuchUpload``. The rule is that the tag, not the shape of the + # exception, is what licenses reading a body as a storage response -- + # so the abort path has to go through ``_make_upload_request`` too. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + abort_url = generate_url_helper(key=path.full_path, method='DELETE', expires=100, + headers={}, query_parameters=params) + error_xml = ('' + 'AccessDeniednope') + aiohttpretty.register_uri('DELETE', abort_url, body=error_xml.encode('utf-8'), status=403) + + seen = [] + original = provider._log_abort_failure + monkeypatch.setattr(provider, '_log_abort_failure', + lambda err, *a, **kw: (seen.append(err), original(err, *a, **kw))[1]) + + await provider._abort_chunked_upload(path, upload_id) + + assert seen + assert all(pd_provider._is_storage_response(err) for err in seen) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_failure_log_omits_raw_body(self, provider, mock_time, + generate_url_helper, caplog): + # ``'{!r}'.format(UploadError(...))`` renders the entire message, and for + # a dict message that is the storage's raw body serialised as JSON -- + # unbounded, and carrying Resource paths. ``_translate_upload_error`` + # has a single bounded log for the body; this one must only identify the + # failure by type and status. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + abort_url = generate_url_helper(key=path.full_path, method='DELETE', expires=100, + headers={}, query_parameters=params) + error_xml = ('' + 'AccessDenied' + '/bucket/secret-key-name' + '{}').format('z' * 4096) + aiohttpretty.register_uri('DELETE', abort_url, body=error_xml.encode('utf-8'), + status=403) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + aborted = await provider._abort_chunked_upload(path, upload_id) + + assert aborted is False + failures = [r.getMessage() for r in caplog.records + if r.name == PROVIDER_LOGGER and 'upload_id={}'.format(upload_id) in + r.getMessage() and 'has failed to abort' not in r.getMessage()] + assert failures + for logged in failures: + # Enough to triage with... + assert 'UploadError' in logged + assert str(int(HTTPStatus.FORBIDDEN)) in logged + assert upload_id in logged + # ...including *which* S3 error it was. Removing the raw body + # without carrying the error code over left the log unable to + # distinguish AccessDenied from InternalError, which is the first + # thing anyone reading it needs to know. The code is already + # parsed one line earlier to test for NoSuchUpload. + assert 'AccessDenied' in logged + # ...and nothing of the body itself. + assert 'secret-key-name' not in logged + assert 'zzzz' not in logged + assert len(logged) < 200 + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_abort_chunked_upload_list_empty(self, provider, list_parts_resp_empty, diff --git a/waterbutler/providers/s3compatsigv4/provider.py b/waterbutler/providers/s3compatsigv4/provider.py index 73a78df84..4c1fe86b7 100644 --- a/waterbutler/providers/s3compatsigv4/provider.py +++ b/waterbutler/providers/s3compatsigv4/provider.py @@ -10,6 +10,7 @@ from io import BytesIO import base64 +import aiohttp import xmltodict import boto3 from botocore.config import Config @@ -32,6 +33,66 @@ # namespace-prefixed error documents are not rejected by the cheap pre-filter. ERROR_ELEMENT_RE = re.compile(r'<(?:[^\s:>/]+:)?Error[\s>/]') +# Failures that mean "the exchange with the storage was cut short", as opposed +# to "the storage answered with an error". ``asyncio.TimeoutError`` is NOT a +# subclass of ``aiohttp.ClientError``: the whole-request timeout that +# ``make_request`` applies (``settings.AIOHTTP_TIMEOUT``, 3600s by default) +# raises it directly, so it has to be listed explicitly. This is the exact +# path taken when a storage stops reading the request body on quota exhaustion +# and then goes silent. +CONNECTION_ERRORS = (aiohttp.ClientError, asyncio.TimeoutError) + +# Upper bound on how much of the storage's raw error body is written to the log. +# The body is the only place ``RequestId`` / ``Resource`` survive once the error +# has been translated into a summary message, but it must not be unbounded: a +# misconfigured proxy can answer with a full HTML page. +ERROR_BODY_LOG_LIMIT = 512 + + +# Sentinel for "the key is not in the mapping at all". ``xmltodict`` maps an +# empty element to ``None`` (````, ```` and +# `` `` all become ``{'Error': None}``), so ``None`` on its own +# cannot distinguish "absent" from "present but empty". Those two must not be +# conflated: an empty ```` element still means the request failed. +_MISSING = object() + + +# ``_translate_upload_error`` interprets an error by parsing the storage's XML +# body out of it. That is only meaningful for errors that actually carry one. +# WaterButler also raises ``UploadError`` with its own prose -- e.g. the 502 +# ``_create_upload_session`` raises when the session response is unreadable -- +# and those must pass through untouched: parsing them yields "unclassifiable" +# and the fallback would overwrite the message with a generic one. +# +# The distinction is carried explicitly on the exception rather than inferred +# from its shape. Inferring it (e.g. "``data`` is a dict, so it must be raw") +# is not safe: the two kinds are indistinguishable by inspection, so a newly +# added WaterButler-authored error would silently pick the wrong branch. +_STORAGE_RESPONSE_FLAG = '_wb_storage_response' + +# The failure was observed while committing a multipart upload, so the object +# may exist on the storage even though the request is reported as failed. +_COMMIT_OUTCOME_UNKNOWN_FLAG = '_wb_commit_outcome_unknown' + + +def _mark_storage_response(err): + """Tag ``err`` as carrying a raw storage response body.""" + setattr(err, _STORAGE_RESPONSE_FLAG, True) + return err + + +def _is_storage_response(err): + return getattr(err, _STORAGE_RESPONSE_FLAG, False) + + +def _mark_commit_outcome_unknown(err): + setattr(err, _COMMIT_OUTCOME_UNKNOWN_FLAG, True) + return err + + +def _is_commit_outcome_unknown(err): + return getattr(err, _COMMIT_OUTCOME_UNKNOWN_FLAG, False) + def _local_name_lookup(mapping, local_name, default=None): """Look up ``local_name`` in an ``xmltodict`` mapping, ignoring any XML @@ -117,6 +178,42 @@ def NAME(self): 'interpreted. Please retry the upload, and contact the storage administrator if the ' 'problem persists.' ) + # A dropped connection is only evidence of exhausted capacity, never proof: + # it is just as often a network fault. Naming both keeps the message from + # sending the user off to free up space when nothing is full. + # "before the upload completed" was removed deliberately: the same message + # is returned when the connection drops while *reading the answer to the + # commit*, and there the upload may well have completed. Asserting the + # opposite is what sends the user off to upload the file a second time. + CONNECTION_INTERRUPTED_MESSAGE = ( + 'Upload failed because the connection to the cloud storage was interrupted. This may ' + 'indicate that the storage is full, that its quota has been exceeded, or that there ' + 'was a network problem. Please retry the upload, and contact the storage ' + 'administrator if the problem persists.' + ) + # The commit is still reported as failed (fail-closed), but an unreadable + # answer is not proof that nothing was written. Saying only "the upload + # failed" invites a duplicate upload. + UPLOAD_MAY_HAVE_COMPLETED_MESSAGE = ( + ' The upload may in fact have completed; please check the file list before ' + 'uploading the file again.' + ) + + async def _make_upload_request(self, *args, **kwargs): + """``make_request`` for the upload path, tagging storage-origin failures. + + Every ``UploadError`` that escapes here was built by + ``exception_from_response`` from an actual storage response, so it is + the raw material ``_translate_upload_error`` is allowed to interpret. + Marking at the source keeps that judgement next to the request that + justifies it, instead of re-deriving it from the exception's shape at + the point of use. + """ + try: + return await self.make_request(*args, **kwargs) + except exceptions.UploadError as err: + _mark_storage_response(err) + raise def __init__(self, auth, credentials, settings, **kwargs): """ @@ -224,24 +321,59 @@ def _check_for_200_error( try to parse response body as a xml. if the xml has an 'Error' element then raise an exception. + The raised exception carries the raw body under ``data['response']``, + which is the same shape :func:`.exceptions.exception_from_response` + builds for a genuine non-2xx XML response. Keeping the two shapes + identical is what lets ``_translate_upload_error`` recognise a quota + failure that S3 reported with HTTP 200 (CompleteMultipartUpload does + this) instead of surfacing a bare HTTP 500. + + The check is deliberately *fail-closed*: once an ``Error`` element is + seen the operation has failed, so being unable to classify it (empty + element, missing ``Code``, unparsable body) still raises. Returning + normally would let ``upload()`` go on to read the *previous* object's + metadata and report a successful upload of data that was never + committed. + :param str response_body: API response body. :param str s3_api_name: S3 API name for logging. :param type exception_type: raise Exception type """ + body = response_body.decode('utf-8', 'replace') \ + if isinstance(response_body, bytes) else response_body + try: # memo: If no element, the parser will raise an ExpatError. result = xmltodict.parse(response_body) except ExpatError: + # Letting ExpatError escape surfaces as a bare HTTP 500 with a + # stack trace: ``_translate_upload_error`` has no ``.message`` to + # work with on it. The storage answered unintelligibly, which is + # an upstream fault, so report HTTP 502 -- consistently with + # ``_create_upload_session``. logger.warning('Couldn\'t parse %s result', s3_api_name) - raise + raise _mark_storage_response( + exception_type({'response': body}, code=HTTPStatus.BAD_GATEWAY)) - if 'Error' in result: - error_code = result['Error'].get('Code', 'Unknown') - logger.warning('%s returned with an error: %s', s3_api_name, error_code) - raise exception_type( - f'{s3_api_name} returned with an error.', - code=HTTPStatus.INTERNAL_SERVER_ERROR, - ) + error = _local_name_lookup(result, 'Error', _MISSING) \ + if isinstance(result, dict) else _MISSING + if error is _MISSING: + return + + error_code = None + if isinstance(error, dict): + code = _local_name_lookup(error, 'Code') + if isinstance(code, str) and code.strip(): + error_code = code.strip() + logger.warning('%s returned with an error: %s', s3_api_name, error_code or 'Unknown') + + # The storage reported a failure inside a 2xx response. Sending the + # request was not something WaterButler got wrong, so the fault is + # attributed upstream: HTTP 502 rather than 500. + # ``_translate_upload_error`` refines this to 507 when the body turns + # out to be a quota rejection. + raise _mark_storage_response( + exception_type({'response': body}, code=HTTPStatus.BAD_GATEWAY)) async def download(self, path, accept_url=False, revision=None, range=None, **kwargs): r"""Returns a ResponseWrapper (Stream) for the specified path @@ -393,13 +525,34 @@ def _is_quota_exhaustion(cls, err): Because the list of vendor-specific quota codes cannot be exhaustive, a response that already carries HTTP 507 counts whatever its code is. """ - if not isinstance(err, exceptions.UploadError): + if not isinstance(err, exceptions.UploadError) or not _is_storage_response(err): return False if err.code == HTTPStatus.INSUFFICIENT_STORAGE: return True error_code = cls._parse_s3_error_body(err)[0] return error_code is not None and error_code in settings.QUOTA_EXCEEDED_ERROR_CODES + @classmethod + def _commit_outcome_note(cls, err): + """The notice to append when the commit's outcome is genuinely unknown. + + Decision: a 4xx is the storage *stating* that it refused the request, so + nothing was assembled and telling the user otherwise would send them + hunting for a file that does not exist. Anything else -- a 5xx, or no + status at all because the connection dropped -- leaves "accepted, then + failed while processing" open, and a silent success there is exactly + what produces a duplicate upload. + + The rule lives here rather than at each mark site so that the three + ways a commit can fail cannot drift apart. + """ + if not _is_commit_outcome_unknown(err): + return '' + code = getattr(err, 'code', None) + if isinstance(code, int) and HTTPStatus.BAD_REQUEST <= code < HTTPStatus.INTERNAL_SERVER_ERROR: + return '' + return cls.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + def _translate_upload_error(self, err, extra_message=''): """Translate a raw :class:`.UploadError` from the storage backend into a user-facing error. @@ -419,14 +572,58 @@ def _translate_upload_error(self, err, extra_message=''): error (e.g. a warning that the multipart-upload abort failed) :rtype: :class:`.UploadError` """ + if not _is_storage_response(err): + # WaterButler authored this message, so there is no storage body to + # interpret and the text is already user-facing. Parsing it would + # yield "unclassifiable" and the fallback below would replace it + # with a generic message -- losing, for example, the instruction to + # have an administrator remove a stale multipart session. + # + # This branch is also where an upload call site that forgot + # ``_make_upload_request`` would land, and it degrades *quietly*: + # quota errors would simply stop becoming 507s, with nothing + # failing. DEBUG rather than WARNING because the legitimate case is + # the common one -- this is a breadcrumb for whoever is asking why a + # 507 did not happen, not an alert. + logger.debug('Passing through an untagged upload error unmodified: ' + 'type=%s status=%s', type(err).__name__, err.code) + if not extra_message: + return err + return exceptions.UploadError( + '{}{}'.format(err.message, extra_message), + code=err.code, + # Rebuilding the exception must not silently re-classify it. + # ``is_user_error`` decides the Sentry level in + # ``server/api/v1/core.py``; defaulting it to ``False`` here + # would promote a user-caused failure to an error-level event + # purely because the abort warning had to be appended. + is_user_error=err.is_user_error, + ) + error_code, error_message = self._parse_s3_error_body(err) is_quota_error = self._is_quota_exhaustion(err) + outcome_note = self._commit_outcome_note(err) + + # The translated error only carries a summary message, so this is the + # single place where the storage's own diagnostics (RequestId, Resource) + # can still be recorded. Both upload paths funnel through here. + # Quota exhaustion is logged one level down: it is an expected, + # user-resolvable failure and should not trip log-based alerting (the + # same reasoning as ``is_user_error`` below). + raw_body = self._raw_error_body(err) + # ``int()`` keeps the rendering stable across Python versions: before + # 3.11 ``'%s' % HTTPStatus.FORBIDDEN`` is ``'HTTPStatus.FORBIDDEN'``. + status = int(err.code) if isinstance(err.code, int) else err.code + log = logger.warning if is_quota_error else logger.error + log('Storage rejected the upload: status=%s code=%s body=%.*s', + status, error_code, ERROR_BODY_LOG_LIMIT, raw_body) if is_quota_error: code_note = ' (storage error code: {})'.format(error_code) \ if error_code is not None else '' return exceptions.UploadError( - '{}{}{}'.format(self.QUOTA_EXCEEDED_MESSAGE, code_note, extra_message), + '{}{}{}{}'.format(self.QUOTA_EXCEEDED_MESSAGE, code_note, outcome_note, + extra_message), code=HTTPStatus.INSUFFICIENT_STORAGE, # Running out of storage is an expected, user-resolvable failure: # keep it out of Sentry's error level and the 5xx alerting path. @@ -435,8 +632,8 @@ def _translate_upload_error(self, err, extra_message=''): if error_code is not None: return exceptions.UploadError( 'Upload failed because the cloud storage returned an error. ' - '(storage error code: {}, message: {}){}'.format( - error_code, error_message, extra_message), + '(storage error code: {}, message: {}){}{}'.format( + error_code, error_message, outcome_note, extra_message), code=err.code, ) # Nothing could be classified. ``err`` must still not be handed back: @@ -445,9 +642,11 @@ def _translate_upload_error(self, err, extra_message=''): # otherwise. For a dict message that body is the storage's raw XML # (Resource paths, RequestId); for the string message that # ``exception_from_response`` builds when there is no body, it is the - # presigned URL including its signature. + # presigned URL including its signature. The raw body is already in + # the log above, which is where it belongs. return exceptions.UploadError( - '{}{}'.format(self.UNCLASSIFIED_STORAGE_ERROR_MESSAGE, extra_message), + '{}{}{}'.format(self.UNCLASSIFIED_STORAGE_ERROR_MESSAGE, outcome_note, + extra_message), code=err.code) async def upload(self, stream, path, conflict='replace', **kwargs): @@ -491,7 +690,7 @@ async def _contiguous_upload(self, stream, path): query_parameters = {'Bucket': self.bucket_name, 'Key': path.full_path} try: - resp = await self.make_request( + resp = await self._make_upload_request( 'PUT', functools.partial( self.connection.generate_presigned_url, @@ -515,6 +714,14 @@ async def _contiguous_upload(self, stream, path): # body, and a chained ``__context__`` puts it straight back into # the rendered traceback. raise self._translate_upload_error(err) from None + except CONNECTION_ERRORS as err: + # Some S3-compatible storages close the connection while the client + # is still sending the request body (e.g. when the storage-side + # quota has been exceeded). Without this handler the raw client + # error propagates as an unexplained HTTP 500. + logger.error('Connection error during contiguous upload: {!r}'.format(err)) + raise exceptions.UploadError(self.CONNECTION_INTERRUPTED_MESSAGE, + code=HTTPStatus.BAD_GATEWAY) # S3-compatible server automatically validates Content-MD5 # If MD5 doesn't match, server returns 400 UploadError before writing data @@ -534,6 +741,10 @@ async def _chunked_upload(self, stream, path): # body, and a chained ``__context__`` puts it straight back into # the rendered traceback. raise self._translate_upload_error(err) from None + except CONNECTION_ERRORS as err: + logger.error('Connection error during multipart session creation: {!r}'.format(err)) + raise exceptions.UploadError(self.CONNECTION_INTERRUPTED_MESSAGE, + code=HTTPStatus.BAD_GATEWAY) try: # Step 2. Break stream into chunks and upload them one by one @@ -542,7 +753,19 @@ async def _chunked_upload(self, stream, path): await self._complete_multipart_upload(path, session_upload_id, parts_metadata) except Exception as err: msg = 'An unexpected error has occurred during the multi-part upload.' - logger.error('{} upload_id={} error={!r}'.format(msg, session_upload_id, err)) + # Type and status only. ``repr()`` of a WaterButlerError renders + # its whole message, and for a dict message that is the storage's + # raw body serialised as JSON -- unbounded, and about to be logged + # again (bounded) by ``_translate_upload_error``. + err_code = getattr(err, 'code', None) + # Quota exhaustion is expected and the user can resolve it, so it + # must not go out at ERROR here after the translator has already + # classified it as a warning -- otherwise this line alone keeps + # paging oncall. + log = logger.warning if self._is_quota_exhaustion(err) else logger.error + log('%s upload_id=%s error_type=%s error_code=%s', msg, session_upload_id, + type(err).__name__, + int(err_code) if isinstance(err_code, int) else err_code) aborted = await self._abort_chunked_upload(path, session_upload_id) abort_message = '' if not aborted: @@ -556,7 +779,17 @@ async def _chunked_upload(self, stream, path): # translate quota-exhaustion responses into a user-facing error. raise self._translate_upload_error( err, extra_message=abort_message) from None - raise exceptions.UploadError('{}{}'.format(msg, abort_message)) + if isinstance(err, CONNECTION_ERRORS): + # A connection error carries no status, so the notice applies + # whenever the commit was the request that dropped. This is + # the path a read failure at commit time takes. + raise exceptions.UploadError( + '{}{}{}'.format(self.CONNECTION_INTERRUPTED_MESSAGE, + self._commit_outcome_note(err), abort_message), + code=HTTPStatus.BAD_GATEWAY, + ) + raise exceptions.UploadError( + '{}{}{}'.format(msg, self._commit_outcome_note(err), abort_message)) async def _create_upload_session(self, path): """This operation initiates a multipart upload and returns an upload ID. This upload ID is @@ -574,7 +807,7 @@ async def _create_upload_session(self, path): query_parameters = {'Bucket': self.bucket_name, 'Key': path.full_path} - resp = await self.make_request( + resp = await self._make_upload_request( 'POST', functools.partial( self.connection.generate_presigned_url, @@ -592,9 +825,34 @@ async def _create_upload_session(self, path): throws=exceptions.UploadError, ) upload_session_metadata = await resp.read() - session_data = xmltodict.parse(upload_session_metadata, strip_whitespace=False) - # Session upload id is the only info we need - return session_data['InitiateMultipartUploadResult']['UploadId'] + try: + session_data = xmltodict.parse(upload_session_metadata, strip_whitespace=False) + # Session upload id is the only info we need + session_upload_id = session_data['InitiateMultipartUploadResult']['UploadId'] + if not isinstance(session_upload_id, str) or not session_upload_id.strip(): + # An empty ```` parses to ``None`` and an attribute-only + # element to a dict. Returning either would make every following + # request use a bogus upload id. + raise ValueError('UploadId is missing or blank') + # ``strip_whitespace=False`` keeps the indentation of a + # pretty-printed body inside the element, and the id goes straight + # into the ``uploadId`` query parameter of every following request + # (and into its SigV4 signature). + return session_upload_id.strip() + except (ExpatError, KeyError, TypeError, ValueError) as err: + # The storage returned 200/201 but the body is not the expected XML. + # NOTE: at this point a multipart session MAY have been created on + # the storage side but its UploadId is unknown, so it cannot be + # aborted here. Log enough information for manual clean-up. + logger.error('Failed to parse the CreateMultipartUpload response: key={} ' + 'error={!r} body={!r}'.format(path.full_path, err, + upload_session_metadata[:512])) + raise exceptions.UploadError( + 'Failed to create a multipart upload session: the cloud storage returned an ' + 'unexpected response. A stale multipart upload session may remain on the ' + 'storage; please ask the storage administrator to check for and remove it.', + code=HTTPStatus.BAD_GATEWAY, + ) async def _upload_parts(self, stream, path, session_upload_id): """Uploads all parts/chunks of the given stream to S3 one by one.""" @@ -626,7 +884,7 @@ async def _upload_part(self, stream, path, session_upload_id, chunk_number, chun 'UploadId': session_upload_id, } - resp = await self.make_request( + resp = await self._make_upload_request( 'PUT', functools.partial( self.connection.generate_presigned_url, @@ -647,6 +905,60 @@ async def _upload_part(self, stream, path, session_upload_id, chunk_number, chun await resp.release() return resp.headers + @staticmethod + def _log_abort_failure(err, session_upload_id, s3_error_code=None): + """Log an abort attempt that failed, by *kind* rather than by content. + + ``'{!r}'.format(err)`` renders ``WaterButlerError.__repr__``, which + embeds the whole message -- and for an error built from a storage + response that message is the raw body serialised as JSON. It is + unbounded, it carries the storage's ``Resource`` paths, and it is + emitted once per retry. ``_translate_upload_error`` already logs the + body once, bounded by ``ERROR_BODY_LOG_LIMIT``, so repeating it here + buys nothing. The type and status are what actually identify the + failure when reading the log. + + Dropping the body still has to leave the log able to say *which* S3 + error this was. The caller has already parsed the code to test for + ``NoSuchUpload``, so passing it on costs nothing; ``error_code=`` + then means the same thing here as it does in the entry log, and the + HTTP status gets its own name. + """ + status = getattr(err, 'code', None) + logger.error('An unexpected error has occurred during the aborting a multipart ' + 'upload. upload_id={} error_type={} status={} error_code={}'.format( + session_upload_id, type(err).__name__, + int(status) if isinstance(status, int) else status, s3_error_code)) + + @staticmethod + def _no_parts_left(resp_xml): + """Whether a LIST PARTS body reports an empty parts list.""" + uploaded_chunks_list = xmltodict.parse(resp_xml, strip_whitespace=False) + return len(uploaded_chunks_list['ListPartsResult'].get('Part', [])) == 0 + + async def _abort_confirmed_by_list_parts(self, path, session_upload_id): + """Ask LIST PARTS whether an abort that reported ``NoSuchUpload`` took effect. + + This is the same criterion the success path applies, asked of the same + endpoint -- the point is precisely that it is *not* the DELETE's own + word for it. + + A failure to obtain the answer returns ``False``. The whole reason this + check exists is that declaring success on an unestablished claim is what + suppresses the "please remove the parts manually" warning; an + unanswerable question has to keep the claim unestablished, not resolve + it by default. The caller then logs and retries, which is what it would + have done without this branch at all. + """ + try: + resp_xml, session_deleted = await self._list_uploaded_chunks(path, session_upload_id) + return session_deleted or self._no_parts_left(resp_xml) + except Exception as err: + logger.warning('Could not confirm a NoSuchUpload abort via ListParts. ' + 'upload_id={} error_type={}'.format(session_upload_id, + type(err).__name__)) + return False + async def _abort_chunked_upload(self, path, session_upload_id): """This operation aborts a multipart upload. After a multipart upload is aborted, no additional parts can be uploaded using that upload ID. The storage consumed by any @@ -678,7 +990,7 @@ async def _abort_chunked_upload(self, path, session_upload_id): while iteration_count < settings.CHUNKED_UPLOAD_MAX_ABORT_RETRIES: try: # ABORT - resp = await self.make_request( + resp = await self._make_upload_request( 'DELETE', functools.partial( self.connection.generate_presigned_url, @@ -696,20 +1008,35 @@ async def _abort_chunked_upload(self, path, session_upload_id): # LIST PARTS resp_xml, session_deleted = await self._list_uploaded_chunks(path, session_upload_id) - if session_deleted: - # Abort is successful if the session has been deleted + # Abort is successful if the session has been deleted, or when + # there is no part left. + if session_deleted or self._no_parts_left(resp_xml): is_aborted = True break - - uploaded_chunks_list = xmltodict.parse(resp_xml, strip_whitespace=False) - parsed_parts_list = uploaded_chunks_list['ListPartsResult'].get('Part', []) - if len(parsed_parts_list) == 0: - # Abort is successful when there is no part left + except exceptions.UploadError as err: + # ``NoSuchUpload`` means the session is already gone, which is + # exactly the state abort is trying to reach. Retrying to the + # cap and then reporting failure sends the user hunting for + # parts that do not exist -- which is what happens whenever the + # commit actually succeeded and only its response was + # unreadable. + # + # But "the session is gone" is not the same claim as "the parts + # are gone". The docstring's own success criterion is LIST + # PARTS returning 404 or an empty list, and ``NoSuchUpload`` on + # the DELETE does not establish either -- a storage that has + # dropped the session record while parts are still billable + # would answer exactly this way. So confirm it with the same + # LIST PARTS check the success path uses, and fall through to + # log-and-retry when the check disagrees. + s3_error_code = self._parse_s3_error_body(err)[0] + if s3_error_code == 'NoSuchUpload' and \ + await self._abort_confirmed_by_list_parts(path, session_upload_id): is_aborted = True break + self._log_abort_failure(err, session_upload_id, s3_error_code) except Exception as err: - msg = 'An unexpected error has occurred during the aborting a multipart upload.' - logger.error('{} upload_id={} error={!r}'.format(msg, session_upload_id, err)) + self._log_abort_failure(err, session_upload_id) iteration_count += 1 @@ -735,7 +1062,7 @@ async def _list_uploaded_chunks(self, path, session_upload_id): 'UploadId': session_upload_id, } - resp = await self.make_request( + resp = await self._make_upload_request( 'GET', functools.partial( self.connection.generate_presigned_url, @@ -784,28 +1111,55 @@ async def _complete_multipart_upload(self, path, session_upload_id, parts_metada 'UploadId': session_upload_id } - resp = await self.make_request( - 'POST', - functools.partial( - self.connection.generate_presigned_url, - 'complete_multipart_upload', - Params=query_parameters, - ExpiresIn=200, - HttpMethod='POST', - ), - data=payload, - headers=headers, - expects=( - HTTPStatus.OK, - HTTPStatus.CREATED, - ), - throws=exceptions.UploadError, - ) - - response_body = await resp.read() - self._check_for_200_error(response_body, "CompleteMultipartUpload", exceptions.UploadError) + # Every failure from here on is a failure of the commit itself, and the + # commit is the one request whose outcome WaterButler cannot infer: + # the parts are already stored, so "did the assemble happen?" is + # answered only by the response. Marking at each of the three places + # the commit can fail is what lets ``_translate_upload_error`` tell the + # user before they upload the file a second time. Which marks actually + # produce the notice is decided there, not here. + try: + resp = await self._make_upload_request( + 'POST', + functools.partial( + self.connection.generate_presigned_url, + 'complete_multipart_upload', + Params=query_parameters, + ExpiresIn=200, + HttpMethod='POST', + ), + data=payload, + headers=headers, + expects=( + HTTPStatus.OK, + HTTPStatus.CREATED, + ), + throws=exceptions.UploadError, + ) + except Exception as err: + _mark_commit_outcome_unknown(err) + raise - await resp.release() + try: + # ``read()`` belongs inside the try: a connection dropped mid-body + # is exactly what a struggling storage does, and leaving the read + # outside meant that case skipped the release entirely. + response_body = await resp.read() + # S3 reports CompleteMultipartUpload failures as HTTP 200 plus an + # body, so this raises on what looks like a success -- and + # that is exactly the path a quota-exhausted storage takes for + # every upload. Without the finally, each one leaks a connection. + self._check_for_200_error(response_body, "CompleteMultipartUpload", + exceptions.UploadError) + except Exception as err: + # ``Exception`` rather than ``UploadError``: the storage did answer + # the commit and we could not read the answer, which arrives as an + # aiohttp error. Narrowing this to ``UploadError`` meant the one + # case the notice exists for was the one case that never got it. + _mark_commit_outcome_unknown(err) + raise + finally: + await resp.release() async def move(self, dest_provider, src_path, dest_path, rename=None, conflict='replace', handle_naming=True): From 35e379270cf2e8d1d3201dab888fa2fb59f117ab Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 16 Sep 2026 21:28:37 +0900 Subject: [PATCH 5/8] =?UTF-8?q?fix(s3compatsigv4):=20=E8=A8=AD=E5=AE=9A?= =?UTF-8?q?=E5=80=A4=E3=81=8C=E4=B8=8D=E6=AD=A3=E3=81=A7=E3=82=82=E3=83=97?= =?UTF-8?q?=E3=83=AD=E3=83=90=E3=82=A4=E3=83=80=E3=81=94=E3=81=A8=E8=90=BD?= =?UTF-8?q?=E3=81=A8=E3=81=95=E3=81=AA=E3=81=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `QUOTA_EXCEEDED_ERROR_CODES` は provider config で拡張できるが、値が 不正な JSON だと import 時に例外が出る。これは静かな障害になる。stevedore が 読み込み失敗を握って `ProviderNotFound` に変換するため、プロセスは生き残り、 s3compatsigv4 だけが全リクエストに HTTP 404 を返す状態になるためである。 不正な JSON、スカラー、`null`、マッピングをそれぞれ警告つきで正規化し、 既定値に倒して起動を続ける。設定値は最終的に `frozenset` の文字列集合となり、 コード照合が部分一致や1文字ずつの比較に化けることを防ぐ。 --- .../providers/s3compatsigv4/test_provider.py | 142 ++++++++++++++++++ .../providers/s3compatsigv4/settings.py | 85 ++++++++++- 2 files changed, 225 insertions(+), 2 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index a5ee786cb..4ead279fa 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -8,6 +8,7 @@ import asyncio import logging import datetime +import importlib import aiohttp import aiohttpretty @@ -1779,6 +1780,110 @@ def test_translate_upload_error_quota_logged_as_warning(self, provider, caplog): assert records[0].levelno == logging.WARNING assert 'QUOTAREQUESTID' in records[0].getMessage() + def test_quota_exceeded_error_codes_env_override(self): + # ``SettingsDict.get`` always returns a ``str`` when the envvar is set, + # which turns the ``error_code in ...`` membership test into substring + # matching. List settings must be read with ``get_object``. + env = {'S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES': + '["XMinioStorageFull"]'} + try: + with mock.patch.dict(os.environ, env): + codes = importlib.reload(pd_settings).QUOTA_EXCEEDED_ERROR_CODES + finally: + # Restore the module *after* the envvar patch is undone, otherwise + # the override leaks into every later test. + importlib.reload(pd_settings) + + assert not isinstance(codes, str) + assert 'XMinioStorageFull' in codes + # A substring of a configured code must never be treated as a match. + assert 'StorageFull' not in codes + + @pytest.mark.parametrize('label,raw,expected', [ + # ``get_object`` is ``json.loads`` with no type check, so the envvar can + # legitimately decode to any JSON type. Every one of them has to end up + # as a set of strings, because the only consumer is ``code in codes``. + ('json-array', '["XMinioStorageFull"]', {'XMinioStorageFull'}), + # A quoted JSON scalar decodes to ``str``. Feeding that to ``frozenset`` + # explodes it into one entry per character, so the configured code stops + # matching entirely -- and nothing fails loudly. + ('json-scalar-string', '"QuotaExceeded"', {'QuotaExceeded'}), + # A JSON number is not iterable at all: ``frozenset(507)`` raises + # ``TypeError`` while the settings module is being imported, which takes + # the whole provider down rather than just mis-classifying an error. + ('json-number', '507', {'507'}), + ]) + def test_quota_exceeded_error_codes_env_types(self, label, raw, expected): + # This must exercise the *real* path: envvar -> ``get_object`` -> + # whatever normalisation the settings module does. Patching the + # already-computed attribute would skip exactly the code under test. + env = {'S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES': raw} + try: + with mock.patch.dict(os.environ, env): + codes = importlib.reload(pd_settings).QUOTA_EXCEEDED_ERROR_CODES + finally: + importlib.reload(pd_settings) + + assert set(codes) == expected + # A substring of a configured code must never be treated as a match. + for code in expected: + assert code[:-1] not in codes + + @pytest.mark.parametrize('label,raw', [ + # The value operators are most likely to write: the bare error code, + # without the JSON quoting ``get_object`` requires. + ('bare-word', 'QuotaExceeded'), + ('comma-separated', 'QuotaExceeded,XMinioStorageFull'), + ('empty-string', ''), + ('truncated-json', '["QuotaExceeded"'), + ]) + def test_malformed_json_falls_back_instead_of_killing_the_import(self, label, raw): + # ``_normalise_error_codes`` is applied to the *return value* of + # ``get_object``, so it never sees a value that ``json.loads`` refused. + # An import-time ``JSONDecodeError`` is not a loud failure: stevedore + # turns the entry-point load error into ``ProviderNotFound``, so every + # s3compatsigv4 request answers 404 while the process stays up and the + # other providers keep working. A quota-code typo must not do that. + env = {'S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES': raw} + try: + with mock.patch.dict(os.environ, env): + codes = importlib.reload(pd_settings).QUOTA_EXCEEDED_ERROR_CODES + finally: + importlib.reload(pd_settings) + + # Falling back to the defaults keeps quota detection working rather + # than leaving it configured with a half-parsed value. + assert 'QuotaExceeded' in codes + assert 'XMinioStorageFull' in codes + + def test_mapping_config_is_rejected_rather_than_silently_degraded(self): + # A ``dict`` satisfies ``Iterable``, so it slips past the scalar branch + # and ``frozenset(str(code) for code in ...)`` quietly reduces it to its + # *keys*. That is indistinguishable from a working configuration until + # a quota error fails to be recognised in production. + env = {'S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES': '{"QuotaExceeded": 507}'} + try: + with mock.patch.dict(os.environ, env): + codes = importlib.reload(pd_settings).QUOTA_EXCEEDED_ERROR_CODES + finally: + importlib.reload(pd_settings) + + assert 'XMinioStorageFull' in codes + + def test_quota_error_codes_env_types_reach_the_provider(self, provider): + # The normalisation is only useful if the value the provider actually + # reads is the normalised one. + env = {'S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES': '"QuotaExceeded"'} + try: + with mock.patch.dict(os.environ, env): + importlib.reload(pd_settings) + translated = provider._translate_upload_error(storage_error( + {'response': 'QuotaExceeded'}, code=403)) + finally: + importlib.reload(pd_settings) + + assert translated.code == HTTPStatus.INSUFFICIENT_STORAGE + def test_quota_exceeded_error_codes_defaults(self): codes = pd_settings.QUOTA_EXCEEDED_ERROR_CODES # MinIO returns XMinioStorageFull on the S3 data path when the disk is @@ -1789,6 +1894,43 @@ def test_quota_exceeded_error_codes_defaults(self): # Not an S3 error code -- it is an HTTP reason phrase. assert 'InsufficientStorage' not in codes + @pytest.mark.parametrize('label,configured,expected', [ + ('list', ['QuotaExceeded'], {'QuotaExceeded'}), + # A bare ``str`` must be wrapped, not iterated: iterating it yields one + # entry per character and ``'Quota' in 'QuotaExceeded'`` would have + # turned the membership test into substring matching. + ('bare-string', 'QuotaExceeded', {'QuotaExceeded'}), + # A JSON number is not iterable, so it has to be wrapped before the + # ``frozenset`` call rather than after it. + ('bare-int', 507, {'507'}), + ('mixed-list', ['QuotaExceeded', 507], {'QuotaExceeded', '507'}), + ('tuple', ('QuotaExceeded',), {'QuotaExceeded'}), + ]) + def test_normalise_error_codes(self, label, configured, expected): + codes = pd_settings._normalise_error_codes(configured) + + assert isinstance(codes, frozenset) + assert codes == expected + # Every element is a ``str``, so ``code in codes`` can never raise. + assert all(isinstance(code, str) for code in codes) + + @pytest.mark.parametrize('configured,warns', [ + (['QuotaExceeded'], False), + ('QuotaExceeded', True), + (507, True), + ]) + def test_normalise_error_codes_warns_on_scalar(self, caplog, configured, warns): + # A scalar is coerced, not rejected: raising here would happen at import + # time and take the provider down over a typo. The warning is the only + # signal the operator gets, so it must actually be emitted. + with caplog.at_level(logging.WARNING, logger=pd_settings.__name__): + pd_settings._normalise_error_codes(configured) + + records = [r for r in caplog.records if r.name == pd_settings.__name__] + assert bool(records) is warns + if warns: + assert 'QUOTA_EXCEEDED_ERROR_CODES' in records[0].getMessage() + def test_translate_upload_error_507_fallback(self, provider): # The storage may answer 507 with an error code we do not know. The # status alone is enough to treat it as a quota failure. diff --git a/waterbutler/providers/s3compatsigv4/settings.py b/waterbutler/providers/s3compatsigv4/settings.py index b6dd1df69..19b1172b6 100644 --- a/waterbutler/providers/s3compatsigv4/settings.py +++ b/waterbutler/providers/s3compatsigv4/settings.py @@ -1,5 +1,10 @@ +import logging +from collections.abc import Iterable, Mapping + from waterbutler import settings +logger = logging.getLogger(__name__) + config = settings.child('S3COMPAT_PROVIDER_CONFIG') @@ -22,8 +27,84 @@ # vendor uses a different error code. ``get_object`` is required here: plain # ``get`` returns the raw string when the value comes from an envvar, which # would silently turn the membership test into substring matching. -QUOTA_EXCEEDED_ERROR_CODES = frozenset(config.get_object('QUOTA_EXCEEDED_ERROR_CODES', [ + + +QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS = [ 'QuotaExceeded', 'XMinioAdminBucketQuotaExceeded', 'XMinioStorageFull', -])) +] + + +def _read_error_codes(): + """Read the configured error-code list, surviving anything the envvar holds. + + ``SettingsDict.get_object`` calls ``json.loads`` with no ``try``, so an + envvar that is not valid JSON raises while this module is being imported. + Normalising ``get_object``'s *return value* -- which is what + ``_normalise_error_codes`` does -- cannot help, because the argument is + evaluated first. + + An import failure here is not the loud failure it looks like. stevedore + turns the entry-point load error into a ``RuntimeError``, which + ``waterbutler/core/utils.py`` converts into ``ProviderNotFound``: the + process stays up, every other provider keeps working, and s3compatsigv4 + answers HTTP 404 to everything. A typo in a quota code would surface as a + symptom with no visible connection to its cause. Falling back to the + defaults keeps quota detection working and puts the cause in the log. + """ + try: + return config.get_object('QUOTA_EXCEEDED_ERROR_CODES', + QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS) + except (ValueError, TypeError) as err: + # ``JSONDecodeError`` is a ``ValueError``. ``TypeError`` covers a + # non-string, non-JSON value arriving from a config file. + logger.warning('S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES is not valid JSON ' + '(%s: %s); falling back to the built-in defaults. A bare error code ' + 'must be quoted, e.g. \'["QuotaExceeded"]\'.', + type(err).__name__, err) + return QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS + + +def _normalise_error_codes(configured): + """Coerce a configured error-code list into a ``frozenset`` of ``str``. + + ``SettingsDict.get_object`` is ``json.loads`` with no type check + (``waterbutler/settings.py``), so the value can decode to any JSON type. + The only consumer is a ``code in codes`` membership test, which degrades + silently for every type but a collection of strings: + + * a bare ``str`` makes the test substring matching (``'Quota'`` would match + a configured ``'QuotaExceeded'``) -- and handing it to ``frozenset`` + instead explodes it into one entry per character, so nothing matches; + * a number is not iterable at all, so ``frozenset`` raises ``TypeError``. + + Normalising here rather than at the point of use is deliberate: this module + is the only place the raw configuration exists, so fixing the type here + means no caller can observe the un-normalised value. + + A scalar is coerced rather than rejected. Raising would happen at import + time and take the whole provider down over a quota-code typo, which is a + far worse outcome than running with the single code the operator meant; + the warning is what makes the misconfiguration visible. + + A mapping is rejected rather than coerced. ``dict`` satisfies ``Iterable``, + so it would slip past the scalar branch and be reduced to its *keys* -- + a result that is indistinguishable from a working configuration until a + quota error goes unrecognised in production. There is no reading of + ``{"QuotaExceeded": 507}`` that makes the operator's intent unambiguous. + """ + if isinstance(configured, Mapping): + logger.warning('S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES is a mapping; ' + 'expected a list of error codes. Falling back to the built-in ' + 'defaults.') + configured = QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS + elif isinstance(configured, str) or not isinstance(configured, Iterable): + logger.warning('S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES is a scalar ' + '(%s); expected a list. Treating it as a single error code.', + type(configured).__name__) + configured = (configured,) + return frozenset(str(code) for code in configured) + + +QUOTA_EXCEEDED_ERROR_CODES = _normalise_error_codes(_read_error_codes()) From 96b3af612ad642d32511ee2b476cfbb141d93ef8 Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 16 Sep 2026 21:28:52 +0900 Subject: [PATCH 6/8] =?UTF-8?q?fix(s3compatsigv4):=20=E3=82=B3=E3=83=9F?= =?UTF-8?q?=E3=83=83=E3=83=88=E6=88=90=E5=90=A6=E3=81=8C=E4=B8=8D=E6=98=8E?= =?UTF-8?q?=E3=81=AA=E3=81=A8=E3=81=8D=E3=81=AF=E3=81=9D=E3=81=AE=E6=97=A8?= =?UTF-8?q?=E3=82=92=E4=BC=9D=E3=81=88=E3=82=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commit(`CompleteMultipartUpload`)が失敗したとき、オブジェクトが存在するのか しないのかを断定できない場合がある。誤った断定は利用者を誤った復旧操作に導く。 成否を3値で扱う。エラーコードが確定的な拒否を保証するときは `NOT_COMMITTED` として注記を付けない。保証できないとき(不定コード / 応答が読めない / 接続断)は `UNKNOWN` として「完了している可能性がある」旨の注記を付ける。判定は S3 の エラーコードの分類表のみで行い、HTTP ステータス階級や輸送経路には依存させない。 迷った場合は `UNKNOWN` に倒す(過剰な注記は許容、誤った断定は不可)。 前提として commit の POST がソケット上でちょうど1回であることが必要なため、 `retry=0` で core の再送を、`allow_redirects=False` で aiohttp 既定の 307/308 自動再 POST を止める。どちらを外してもテストが落ちるようにしてある。 注記の判定は単一の純粋関数に集約し、中止時の `NoSuchUpload` は鵜呑みにせず `ListParts` で確認する。ログに残す応答本文は 512 バイトで切る。 --- .../providers/s3compatsigv4/test_provider.py | 1354 ++++++++++++++++- .../providers/s3compatsigv4/provider.py | 390 ++++- .../providers/s3compatsigv4/settings.py | 21 +- 3 files changed, 1677 insertions(+), 88 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index 4ead279fa..8fdd543f4 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -12,6 +12,8 @@ import aiohttp import aiohttpretty +import xmltodict +from aiohttp import web from http import client from http import HTTPStatus from urllib import parse @@ -59,6 +61,170 @@ def storage_error(message, code=403, exception_type=exceptions.UploadError): ) from hmac import compare_digest +# --- commit 成否注記の全直積テストの素材 --------------------------- +# +# 不変条件は「同じ操作文脈・同じ *観測* コードなら、輸送経路が違っても同じ結果」。 +# 経路ごとにパラメータ集合が重ならないと矛盾が表に出ないので、経路と +# コードの直積を1本で張る。 +# +# コードは分類表の代表8種: 確定拒否(注記なし)3件 / 不定3件 / 未知コード / +# コードなし。未知コードとコードなしは **UNKNOWN に倒す**(fail-safe)。 +COMMIT_CODE_CASES = [ + ('AccessDenied', False), + ('InvalidPart', False), + ('EntityTooSmall', False), + ('InternalError', True), + ('SlowDown', True), + ('RequestTimeout', True), + ('XVendorMystery', True), + (None, True), +] + +# 確定拒否コードの分類表そのもの。実装(``DEFINITIVE_REJECTION_CODES``)からは +# 生成せず、独立に書き下す。実装から生成すると、行を消す変更で +# パラメータごと消えてしまい、その行を守るテストが存在しなくなる。 +DEFINITIVE_REJECTION_CODES = [ + 'AccessDenied', + 'InvalidPart', + 'InvalidPartOrder', + 'EntityTooSmall', + 'EntityTooLarge', + 'MalformedXML', + 'SignatureDoesNotMatch', + 'InvalidAccessKeyId', + 'NoSuchBucket', +] + +# コードが観測できる経路。3 x 8 = 24 セル。 +OBSERVED_TRANSPORTS = ['direct_4xx', 'direct_5xx', 'complete_200_error'] +# コードが観測できない経路。2 x 8 = 16 セル。ストレージが何を言うつもりでも +# WaterButler には届かないので、意図したコードに関わらず UNKNOWN になる。 +LATENT_TRANSPORTS = ['disconnect', 'broken_xml'] + + +def commit_error_xml(error_code): + """CompleteMultipartUpload に対する S3 のエラー本文。 + + ``error_code`` が ``None`` のときは ```` を欠いた本文を返す + —— 解析はできるがコードが無い、という「コードなし」セルの実体である。 + """ + if error_code is None: + return ('' + 'boom') + return ('' + '{}boom'.format(error_code)) + + +def arrange_commit_failure(provider, transport, error_code): + """``_complete_multipart_upload`` だけを ``transport`` の形で失敗させる。""" + if transport == 'direct_4xx': + provider.make_request = MockCoroutine(side_effect=exceptions.UploadError( + {'response': commit_error_xml(error_code)}, code=400)) + elif transport == 'direct_5xx': + provider.make_request = MockCoroutine(side_effect=exceptions.UploadError( + {'response': commit_error_xml(error_code)}, code=500)) + elif transport == 'complete_200_error': + # S3 は CompleteMultipartUpload の失敗を HTTP 200 + で返す。 + resp = mock.Mock() + resp.read = MockCoroutine(return_value=commit_error_xml(error_code).encode('utf-8')) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + elif transport == 'disconnect': + # 応答が届いていないのでコードは観測できない。aiohttp の例外が持つ + # ``message`` を本文と取り違えて拾う実装を殺すため、あえて S3 の + # エラー XML を message に入れる(``_raw_error_body`` は ``message`` + # にフォールバックする)。 + provider.make_request = MockCoroutine( + side_effect=aiohttp.ServerDisconnectedError(commit_error_xml(error_code))) + elif transport == 'broken_xml': + # 本文は届いたが途中で切れている。コード文字列は本文中に存在するが + # 解析できないので観測はできない —— 部分一致で拾ってはならない。 + truncated = commit_error_xml(error_code)[:-12].encode('utf-8') + resp = mock.Mock() + resp.read = MockCoroutine(return_value=truncated) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + else: # pragma: no cover - パラメータの打ち間違いを黙って通さない + raise AssertionError('unknown transport: {}'.format(transport)) + + +def arrange_chunked_commit(provider): + """commit だけが失敗する ``_chunked_upload`` の下ごしらえ。""" + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + +class commit_server: + """commit を1本だけ受ける ``aiohttp.web`` サーバ。 + + ``aiohttpretty`` は ``ClientSession._request`` の *上* で応答を差し込むため、 + その呼び出しの *内側* で起きる **リダイレクト追随** は再現できない。 + それを固定するテストは実ソケットを使うしかない。 + + デコードできない応答本文のほうは ``aiohttpretty`` でも再現できる —— + 本文は素通しされ、core の ``exception_from_response`` が同じ + ``UnicodeDecodeError`` を出す。それでも実ソケットで1本張るのは、モック + 注入の3セルが寄りかかっている「注入点が正しい」という前提ごと、実 + ``ClientResponse`` で固定するためであって、再現不能だからではない。 + + 起動から後始末までを丸ごと引き受ける。``runner.setup()`` から URL の + 組み立てまでを ``finally`` の外に置くと、サーバを起動したあとに準備が + 失敗したとき、待ち受けソケットと provider のセッションが開いたまま + 次のテストへ持ち越される。 + + ``runner.setup()`` 自体は守っていない。aiohttp 3.6.2 の + ``BaseRunner.cleanup()`` は ``self._server is None`` のとき即 return する + ので、setup が落ちた状態で呼んでも何もしない。ここで使う ``Application`` + は ``on_startup`` / ``on_cleanup`` を1つも登録しないため、setup が途中で + 落ちて資源が残る経路そのものが無い。 + """ + + def __init__(self, provider, app): + self.provider = provider + self.app = app + self.runner = web.AppRunner(app) + self.url = None + + async def __aenter__(self): + await self.runner.setup() + try: + site = web.TCPSite(self.runner, '127.0.0.1', 0) + await site.start() + # aiohttp 3.6.2 はバインド済みポートをここでしか公開しない。 + # 将来この私有属性が消えたら AttributeError で落ちる —— 黙って + # skip されるより、テストが壊れたことが分かるほうがよい。 + sockets = site._server.sockets + assert sockets, 'the test server bound no socket' + self.url = 'http://127.0.0.1:{}/first'.format(sockets[0].getsockname()[1]) + except Exception: + # ``__aenter__`` が投げると ``__aexit__`` は呼ばれない。 + await self.runner.cleanup() + raise + return self + + async def __aexit__(self, *exc_info): + first = None + try: + # 1本の close が失敗しても、残りのセッションは閉じる。for を + # 素通しにすると、先頭が投げた時点で後続のセッションが開いたまま + # 次のテストへ残る。 + for session in self.provider.session_list: + try: + await session.close() + except Exception as err: + # 伝えるのは最初の失敗だけ。後続は握り潰さず、閉じきる。 + first = first if first is not None else err + finally: + # セッションの close が失敗しても、待ち受けソケットは必ず畳む。 + await self.runner.cleanup() + if first is not None: + raise first + return False + + @pytest.fixture def base_prefix(): return '' @@ -1230,6 +1396,122 @@ async def test_chunked_upload_aborted_success(self, provider, file_stream, mock_ # The parts upload failed, so the commit step must never be attempted. provider._complete_multipart_upload.assert_not_called() + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_chunked_upload_unexpected_error_is_a_500(self, provider, file_stream, + mock_time): + # Every other raise on this path is a 502, so the bare ``UploadError`` + # at the end looks like a missed ``code=``. It is not: an exception + # that is neither an ``UploadError`` (the storage answered) nor a + # connection error (the link failed) did not come from upstream, and a + # 502 would blame the storage for a defect on this side. Pin it so the + # difference stays a decision rather than an oversight. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(side_effect=ValueError('a bug on this side')) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INTERNAL_SERVER_ERROR + assert exc.value.code != HTTPStatus.BAD_GATEWAY + # Not a storage failure, so none of the upstream-facing wording applies. + assert provider.CONNECTION_INTERRUPTED_MESSAGE not in exc.value.message + assert provider.QUOTA_EXCEEDED_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('fails_at, expect_notice', [ + ('parts', False), + ('commit-request', True), + ('commit-read', True), + ]) + async def test_chunked_upload_500_branch_notices_only_a_commit_failure( + self, provider, file_stream, mock_time, fails_at, expect_notice): + # ``_chunked_upload`` の例外処理には出口が3つある。``UploadError`` と + # ``CONNECTION_ERRORS`` の2つは全直積テストが固定しているが、 + # 「どちらでもない例外」の出口(provider.py の 500 分岐)だけは注記の + # 有無を誰も見ていなかった —— この行の ``_commit_outcome_note(err)`` を + # ``''`` に置き換える変異(N19)が全件緑のまま生き残る。 + # + # 空論ではない。Python 3.6 の ``asyncio.CancelledError`` は + # ``Exception`` の派生でありながら ``CONNECTION_ERRORS`` + # (``aiohttp.ClientError`` と ``asyncio.TimeoutError``)のどちらでも + # ないので、リクエスト実行中のキャンセルはちょうどこの分岐に落ちる。 + # commit の応答読み取り中にキャンセルされれば、その commit が通ったか + # どうかは誰にも分からない = 注記が要る状況そのもの。 + # + # コード軸は取らない。この分岐に来る例外は storage の応答ではないので + # 観測コードは常に ``None`` であり、分類表は最初から関与しない。 + # + # **注入点は ``_complete_multipart_upload`` の外側の境界に置く**。 + # commit メソッドそのものを + # マーク済み例外を投げるモックへ置き換えていたが、それでは + # 「マークが立った例外が来たら注記が出る」までしか固定できない。 + # 実際に印を付けているのは commit 側の ``except Exception`` であり、 + # そこを ``except (UploadError,) + CONNECTION_ERRORS`` へ狭める変異 + # (X18)も、マークを ``isinstance`` ガードの内側へ移す変異(X16)も、 + # 旧方式では 360 件すべて緑のまま生き残った。commit の要求側と + # 応答読取側それぞれに例外を注入すれば、印付けは実コードが行う。 + assert issubclass(asyncio.CancelledError, Exception) + assert not issubclass(asyncio.CancelledError, pd_provider.CONNECTION_ERRORS) + + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + released = [] + + class _AnswerWeCannotRead: + """commit の応答。本文の読み取りだけが中断する。""" + + async def read(self): + raise asyncio.CancelledError('cancelled while reading the commit answer') + + async def release(self): + released.append(True) + + if fails_at == 'parts': + # パート送信中の中断。commit はまだ送られていないので、組み立ては + # 起きていないことが構造的に確定する = 注記は出してはいけない。 + provider._upload_parts = MockCoroutine( + side_effect=asyncio.CancelledError('cancelled during the parts')) + provider._make_upload_request = MockCoroutine() + else: + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"e"'}]) + if fails_at == 'commit-request': + # 要求の発行中に中断。応答が返っていないので、ストレージが + # 組み立てを始めたかどうかは分からない。 + provider._make_upload_request = MockCoroutine( + side_effect=asyncio.CancelledError('cancelled while sending the commit')) + else: + # 応答は返ったが本文を読めなかった。commit が通ったかどうかは + # その本文にしか書かれていない。 + provider._make_upload_request = MockCoroutine( + return_value=_AnswerWeCannotRead()) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INTERNAL_SERVER_ERROR + assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice + if fails_at == 'parts': + # commit は1バイトも出ていない。 + provider._make_upload_request.assert_not_called() + else: + assert provider._make_upload_request.call_count == 1 + # 応答を受け取った経路では、読めなくても接続は返す。 + assert released == ([True] if fails_at == 'commit-read' else []) + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_chunked_upload_abort_failure_appends_warning(self, provider, file_stream, @@ -1521,6 +1803,10 @@ async def test_chunked_upload_parts_timeout(self, provider, file_stream, mock_ti assert provider.CONNECTION_INTERRUPTED_MESSAGE in exc.value.message assert exc.value.is_user_error is False provider._abort_chunked_upload.assert_called_with(path, upload_id) + # The parts never finished uploading, so no commit was ever sent. This + # path *does* consult ``_commit_outcome_note``, so the absence has to be + # asserted here rather than assumed. + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message @pytest.mark.asyncio @pytest.mark.aiohttpretty @@ -1757,12 +2043,108 @@ def test_translate_upload_error_log_truncates_body(self, provider, caplog): logged = [r for r in caplog.records if r.name == PROVIDER_LOGGER][0].getMessage() # Pin the bound itself, not just "shorter than the input": the body is - # cut at exactly ERROR_BODY_LOG_LIMIT characters and no further. + # cut at exactly ERROR_BODY_LOG_LIMIT bytes and no further. assert error_xml[:limit] in logged assert error_xml[:limit + 1] not in logged # Nothing else in the record may reintroduce the rest of the body. assert len(logged) < limit * 2 + def test_translate_upload_error_log_bound_is_in_bytes(self, provider, caplog): + # The constant is declared as "how much of the raw error body is written + # to the log", and the scenario its comment names -- a misconfigured + # proxy answering with an HTML page -- is measured in bytes. A + # character-based cut lets a multibyte body through at three times the + # declared size, which is exactly the case a non-English deployment + # hits first. + limit = 512 + assert pd_provider.ERROR_BODY_LOG_LIMIT == limit + error_xml = 'AccessDenied{}'.format( + '\u3042' * limit) + err = storage_error({'response': error_xml}, code=HTTPStatus.FORBIDDEN) + + with caplog.at_level(logging.WARNING, logger=PROVIDER_LOGGER): + provider._translate_upload_error(err) + + logged = [r for r in caplog.records if r.name == PROVIDER_LOGGER][0].getMessage() + # The body contribution is bounded in bytes, so it cannot exceed the + # limit however wide the characters are. (The prefix the log line adds + # is ASCII and well under 512 bytes.) + assert len(logged.encode('utf-8')) < limit * 2 + + @pytest.mark.parametrize('body', [ + # 実測 **1536 バイト**(宣言の3倍)。``replace`` は + # デコード不能な 1 バイトごとに U+FFFD を置き、U+FFFD は UTF-8 で + # 3 バイトある。バイト単位で切ってからデコードするだけでは足りない。 + b'\xff' * 512, + b'\xff' * 4096, + # 有効な多バイト列。切断点が文字の途中に来ると +1〜+2 バイトになる。 + '\u3042' * 512, + ('\u3042' * 512).encode('utf-8'), + # 不正バイトと有効文字が混ざった、実際のプロキシ応答に近い形。 + b'' + b'\xc3\x28' * 300 + '\u3042'.encode('utf-8') * 100, + # 上限未満はそのまま通ること(切りすぎていないことの対照)。 + b'AccessDenied', + 'AccessDenied', + b'', + '', + ]) + def test_bounded_body_never_exceeds_the_declared_limit(self, body): + # 定数の宣言は「生エラー本文の先頭 ERROR_BODY_LOG_LIMIT **バイト**」。 + # 宣言が守られているかは、戻り値を UTF-8 で**再計量**して初めて分かる。 + # 既存テストはログ行全体を `limit * 2` 未満で見ていたため、3倍に膨らむ + # 入力(不正バイト列)を見逃していた。 + bounded = pd_provider._bounded_body(body) + + assert len(bounded.encode('utf-8')) <= pd_provider.ERROR_BODY_LOG_LIMIT + # 切りすぎの検出。判定は**デコード後**の大きさで行う。入力バイト数で + # 書くと `b'\xff' * 512`(入力 512 バイト / デコード後 1536 バイト)で + # 上の表明と両立せず、テスト自体が充足不能になる。 + source = body if isinstance(body, bytes) else body.encode('utf-8') + decoded = source.decode('utf-8', 'replace') + if len(decoded.encode('utf-8')) <= pd_provider.ERROR_BODY_LOG_LIMIT: + assert bounded == decoded + else: + # 上限超過側で大きさしか見ないと、何も残さず ``''`` を返す + # 実装でも通ってしまう。上限を守ることと本文を残すことは別の + # 要求で、``_bounded_body`` の存在理由は後者にある(調査のために + # 先頭を読ませる)。 + # + # 「先頭であること」を接頭辞で固定する。何バイトで切れるかは + # 文字幅と不正バイトの位置で変わるので長さは指定しない。 + assert bounded + assert decoded.startswith(bounded) + # 上限の大半を使い切っていること。1文字だけ返す実装を落とす。 + # U+FFFD / 3バイト文字でも 1/3 は必ず埋まる。 + assert len(bounded.encode('utf-8')) > pd_provider.ERROR_BODY_LOG_LIMIT // 3 + + @pytest.mark.parametrize('body, expected', [ + # 不正バイト列。``replace`` が 1 バイトごとに U+FFFD を置き、U+FFFD は + # UTF-8 で 3 バイトなので、512 バイトに収まるのは 512 // 3 = 170 文字。 + (b'\xff' * 512, '\ufffd' * 170), + # 入力がいくら長くても、残る量は同じ。 + (b'\xff' * 4096, '\ufffd' * 170), + # 有効な3バイト文字。端数の 2 バイトは2段目の ``ignore`` が落とす。 + ('\u3042' * 512, '\u3042' * 170), + (('\u3042' * 512).encode('utf-8'), '\u3042' * 170), + ]) + def test_bounded_body_keeps_exactly_the_leading_bytes(self, body, expected): + # 上限超過側を「接頭辞であること」と「上限の 1/3 より大きいこと」 + # だけで見ると、下限が 170 バイトなので、多バイト本文だけを 256 + # バイトへ切り詰める実装(= 現行の保持量のおよそ半分)でも + # 全件通ってしまう。 + # + # 代表入力については独立した**完全な期待値**を置く。170 という数は + # 実装から導かず、「U+FFFD は3バイト / 上限は512バイト」という宣言から + # 手で計算したものである。 + assert pd_provider.ERROR_BODY_LOG_LIMIT == 512 + assert pd_provider._bounded_body(body) == expected + + def test_bounded_body_passes_none_through(self): + # 本文が無い応答(``exception_from_response`` が本文なしで作った + # エラー)では ``None`` が来る。``''`` に潰すと、ログ上で + # 「本文が空だった」と「本文が無かった」が区別できなくなる。 + assert pd_provider._bounded_body(None) is None + def test_translate_upload_error_quota_logged_as_warning(self, provider, caplog): # Quota exhaustion is an expected, user-resolvable failure (see the # is_user_error handling), so it must not be logged at ERROR level and @@ -1856,6 +2238,43 @@ def test_malformed_json_falls_back_instead_of_killing_the_import(self, label, ra assert 'QuotaExceeded' in codes assert 'XMinioStorageFull' in codes + def test_malformed_json_warns(self, caplog): + # The fallback is silent from the operator's point of view: quota + # detection keeps working with the *defaults*, so the codes they + # configured simply never match. The warning is the only thing that + # connects that symptom to its cause, and asserting the fallback value + # alone does not notice it being demoted to DEBUG. + with caplog.at_level(logging.WARNING, logger=pd_settings.__name__): + env = {'S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES': 'QuotaExceeded'} + with mock.patch.dict(os.environ, env): + pd_settings._read_error_codes() + + records = [r for r in caplog.records if r.name == pd_settings.__name__] + assert len(records) == 1 + assert records[0].levelno == logging.WARNING + assert 'QUOTA_EXCEEDED_ERROR_CODES' in records[0].getMessage() + assert 'not valid JSON' in records[0].getMessage() + + def test_mapping_config_warns(self, caplog): + # Same reasoning as above, for the branch that silently reduced a + # mapping to its keys before R4-B. + with caplog.at_level(logging.WARNING, logger=pd_settings.__name__): + pd_settings._normalise_error_codes({'QuotaExceeded': 507}) + + records = [r for r in caplog.records if r.name == pd_settings.__name__] + assert len(records) == 1 + assert records[0].levelno == logging.WARNING + assert 'mapping' in records[0].getMessage() + + def test_null_config_falls_back_to_the_defaults(self): + # ``null`` is valid JSON, so it reaches ``_normalise_error_codes`` + # intact. ``None`` is not a ``Mapping``, not a ``str`` and not + # ``Iterable``, so the scalar branch wrapped it and produced + # ``frozenset({'None'})`` -- a configuration under which no storage + # error code can ever match. "Unset" is the only sane reading. + assert pd_settings._normalise_error_codes(None) == frozenset( + pd_settings.QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS) + def test_mapping_config_is_rejected_rather_than_silently_degraded(self): # A ``dict`` satisfies ``Iterable``, so it slips past the scalar branch # and ``frozenset(str(code) for code in ...)`` quietly reduces it to its @@ -1931,6 +2350,35 @@ def test_normalise_error_codes_warns_on_scalar(self, caplog, configured, warns): if warns: assert 'QUOTA_EXCEEDED_ERROR_CODES' in records[0].getMessage() + def test_user_facing_messages_keep_their_wording(self, provider): + # Every other assertion in this file spells the expected text as + # ``provider.``, so changing a constant changes the assertion + # with it. Setting one to ``''`` makes ``'' in message`` vacuously true + # and deletes the assertion outright -- measured: emptying + # QUOTA_EXCEEDED_MESSAGE killed zero tests. + # + # Pinning the wording once, here, is what gives those assertions teeth. + # It is deliberately partial (phrases, not the full string) so that + # rewording for clarity stays cheap while deletion and replacement do + # not. This is the same guard H-7 added for + # UPLOAD_MAY_HAVE_COMPLETED_MESSAGE, which had not been carried across + # to the other three constants. + assert 'quota or capacity' in provider.QUOTA_EXCEEDED_MESSAGE + assert 'free up storage space' in provider.QUOTA_EXCEEDED_MESSAGE + + assert 'could not be ' in provider.UNCLASSIFIED_STORAGE_ERROR_MESSAGE + assert 'interpreted' in provider.UNCLASSIFIED_STORAGE_ERROR_MESSAGE + assert 'retry the upload' in provider.UNCLASSIFIED_STORAGE_ERROR_MESSAGE + + assert 'connection to the cloud storage was interrupted' \ + in provider.CONNECTION_INTERRUPTED_MESSAGE + # The message must keep naming the network as a possible cause: a + # dropped connection is evidence of exhausted capacity, never proof. + assert 'network problem' in provider.CONNECTION_INTERRUPTED_MESSAGE + + assert 'may in fact have completed' in provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + assert 'check the file list' in provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + def test_translate_upload_error_507_fallback(self, provider): # The storage may answer 507 with an error code we do not know. The # status alone is enough to treat it as a quota failure. @@ -2145,6 +2593,7 @@ async def test_chunked_upload_session_error_keeps_waterbutler_message( path = WaterButlerPath('/foobah', prepend=provider.prefix) resp = mock.Mock() resp.read = MockCoroutine(return_value=b'this is not the expected xml') + resp.release = MockCoroutine() provider.make_request = MockCoroutine(return_value=resp) with pytest.raises(exceptions.UploadError) as exc: @@ -2267,56 +2716,68 @@ async def test_chunked_upload_complete_read_failure_warns_upload_may_exist( # The connection was still released despite the read blowing up. assert resp.release.called + # 削除: ``test_chunked_upload_complete_notice_follows_status_class`` + # + # 検証対象だった「4xx/5xx のステータスクラス規則」は廃止した。 + # このテストは新しい規則を適用しても 4/4 合格してしまう —— パラメータが + # (403,'AccessDenied') (400,'InvalidPart') (500,'InternalError') + # (503,'SlowDown') と、ステータスクラス分類とコード分類が**たまたま + # 一致する**組み合わせだけで出来ているためである。 + # 廃止済みの規則を検証しつつ緑であるテストは、落ちるテストより危険で、 + # 将来の実装者に「この規則はまだ有効」と誤認させる。よって残さない。 + # 代替は ``test_commit_notice_depends_only_on_the_observed_code`` + # (全直積 24 セル)と ``test_commit_outcome_note_ignores_the_status_class``。 + @pytest.mark.asyncio @pytest.mark.aiohttpretty - @pytest.mark.parametrize('status,error_code,expect_notice', [ - # Decision (C): a 4xx is the storage stating it refused the request, so - # nothing was committed and the notice would be misleading. A 5xx says - # the server failed while handling a request it had already accepted -- - # whether the parts were assembled is genuinely unknown. - (403, 'AccessDenied', False), - (400, 'InvalidPart', False), - (500, 'InternalError', True), - (503, 'SlowDown', True), - ]) - async def test_chunked_upload_complete_notice_follows_status_class( - self, provider, file_stream, mock_time, status, error_code, expect_notice): + async def test_parts_connection_error_does_not_claim_a_commit( + self, provider, file_stream, mock_time): + # This test used to break the connection at *session creation*, which + # raises out of ``_chunked_upload``'s first ``except`` -- a branch that + # never calls ``_commit_outcome_note`` at all. Asserting the notice's + # absence there asserted nothing: the mutation that makes the note + # unconditional left this test green. + # + # The branch that does consult the note is the connection-error arm of + # the outer handler, so break the connection during ``_upload_parts`` + # instead. The parts never finished, so no commit was ever sent and + # the notice must stay off. assert file_stream.size == 6 provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 provider.CHUNK_SIZE = 2 path = WaterButlerPath('/foobah', prepend=provider.prefix) provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') - provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._upload_parts = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) provider._abort_chunked_upload = MockCoroutine(return_value=True) - error_xml = ('' - '{}boom'.format(error_code)) - provider.make_request = MockCoroutine( - side_effect=exceptions.UploadError({'response': error_xml}, code=status)) - with pytest.raises(exceptions.UploadError) as exc: await provider._chunked_upload(file_stream, path) - assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice + assert provider.CONNECTION_INTERRUPTED_MESSAGE in exc.value.message + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message @pytest.mark.asyncio @pytest.mark.aiohttpretty - async def test_create_session_connection_error_does_not_claim_a_commit( + async def test_parts_connection_error_can_claim_a_commit( self, provider, file_stream, mock_time): - # No commit was ever in flight at session creation, so the notice must - # not appear -- otherwise it stops meaning anything. + # Positive control for the test above. Without it, "the notice is + # absent" is indistinguishable from "this branch can never emit the + # notice" -- which is precisely the defect being fixed. assert file_stream.size == 6 provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 provider.CHUNK_SIZE = 2 path = WaterButlerPath('/foobah', prepend=provider.prefix) - provider.make_request = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + dropped = pd_provider._mark_commit_outcome_unknown(aiohttp.ServerDisconnectedError()) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(side_effect=dropped) + provider._abort_chunked_upload = MockCoroutine(return_value=True) with pytest.raises(exceptions.UploadError) as exc: await provider._chunked_upload(file_stream, path) - assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message @pytest.mark.asyncio @pytest.mark.aiohttpretty @@ -2350,6 +2811,478 @@ async def test_chunked_upload_complete_200_with_error_quota(self, provider, file assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message assert 'QuotaExceeded' in exc.value.message provider._abort_chunked_upload.assert_called_with(path, upload_id) + # The storage answered the commit and named the reason, so the outcome + # is *not* unknown. Saying "your quota is exhausted" and "the upload + # may in fact have completed" in the same breath is self-contradictory, + # and it is the combination this exact response produces in production. + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('error_code', [ + # 分類表にあるので抑止される。 + 'AccessDenied', 'EntityTooLarge', + # 分類表には**無い**が、クォータ分岐が先に立つので抑止される + # 。以前はこれを表に載せていたが、実機 MinIO が + # commit 経路でクォータを強制しないことが実測で判明したため外した。 + 'QuotaExceeded', + ]) + async def test_complete_200_with_error_code_suppresses_the_notice( + self, provider, file_stream, mock_time, error_code): + # 主張は「200 経路だから抑止される」ではなく + # 「**コードが分類表にある / クォータ分岐で抑止される**から」に改めた。 + # 旧アサーションは ``_check_for_200_error`` が立てるマーカーを見ており、 + # 経路が抑止していたのかコードが抑止していたのかを区別できなかった。 + # 経路非依存であることは全直積テストが張る。 + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + error_xml = ('' + '{}boom'.format(error_code)) + resp = mock.Mock() + resp.read = MockCoroutine(return_value=error_xml.encode('utf-8')) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('body', [ + # Unparsable: the storage said *something* went wrong but not what. + b'<<< not xml at all', + # An ```` with no usable ``Code`` is equally uninformative. + b'boom', + b' ', + ]) + async def test_complete_200_without_an_error_code_still_warns( + self, provider, file_stream, mock_time, body): + # 抑止が許されるのは、ストレージが実際に判定を下したときだけ。 + # コードが読めなければ commit の成否は本当に分からないので、注記は + # 残さなければならない(``None`` は UNKNOWN)。注記の有無はステータス + # クラスではなくコード単独で決まる。 + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') + provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"etag1"'}]) + provider._abort_chunked_upload = MockCoroutine(return_value=True) + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=body) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('transport', OBSERVED_TRANSPORTS) + @pytest.mark.parametrize('error_code,expect_notice', COMMIT_CODE_CASES) + async def test_commit_notice_depends_only_on_the_observed_code( + self, provider, file_stream, mock_time, transport, error_code, expect_notice): + # 全直積の本体 24 セル。 + # + # 「同じ操作文脈・同じ観測コードなら、経路が違っても同じ結果」を張る。 + # 経路ごとに別のパラメータ集合を使っていたために、3周とも矛盾が + # 表に出なかった —— それがこの直積の存在理由である。 + assert file_stream.size == 6 + arrange_chunked_commit(provider) + path = WaterButlerPath('/foobah', prepend=provider.prefix) + arrange_commit_failure(provider, transport, error_code) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('transport', LATENT_TRANSPORTS) + @pytest.mark.parametrize('latent_code', [code for code, _ in COMMIT_CODE_CASES]) + async def test_commit_notice_when_the_code_cannot_be_observed( + self, provider, file_stream, mock_time, transport, latent_code, monkeypatch): + # 全直積の残り 16 セル。 + # + # 接続断と破損 XML では ``_parse_s3_error_body`` がコードを返せない。 + # ストレージが ``AccessDenied`` を言うつもりだったかどうかは + # WaterButler には分からないので、**意図したコードに関わらず** + # UNKNOWN(注記あり)に倒れなければならない。 + # + # このセルが空振りでないことに注意: 接続断では S3 のエラー XML が + # 例外の ``message`` に、破損 XML ではコード文字列が本文中に、 + # それぞれ *実在する*。コードを本文以外から拾ったり部分一致で拾う + # 実装は、確定拒否 3 コードのセルで注記を落として落ちる。 + assert file_stream.size == 6 + arrange_chunked_commit(provider) + path = WaterButlerPath('/foobah', prepend=provider.prefix) + arrange_commit_failure(provider, transport, latent_code) + + # 「観測できない」がこのセル群の前提そのものなので、結論(注記あり) + # だけでなく前提も直接見る。注記を無条件に出す実装でも結論の + # アサートは通ってしまうが、``_observed_error_code`` が本文中の + # コード文字列を拾い始めたらここで落ちる。 + observed = [] + real_observed_error_code = pd_provider.S3CompatSigV4Provider._observed_error_code + + def spy(err): + code = real_observed_error_code(err) + observed.append(code) + return code + + monkeypatch.setattr(pd_provider.S3CompatSigV4Provider, + '_observed_error_code', staticmethod(spy)) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message + assert observed and all(code is None for code in observed) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('transport', OBSERVED_TRANSPORTS) + @pytest.mark.parametrize('error_code', ['QuotaExceeded', + 'XMinioAdminBucketQuotaExceeded', + 'XMinioStorageFull']) + async def test_quota_branch_suppresses_the_notice_on_every_transport( + self, provider, file_stream, mock_time, transport, error_code): + # クォータコードは分類表には載せない(実機 MinIO は commit 経路で + # クォータを強制しないことが実測で判明したため、「未コミットの保証」と + # しては使えない)。代わりに**クォータ分岐が分類表より先に立ち**、 + # 注記を抑止する。「容量が足りません」と「完了しているかもしれません」の + # 連結こそが、この PR が直しているバグの本体である。 + # + # 200 経路だけを見ていると、その経路ではマーカー側ですでに注記が + # '' になっているため、クォータ分岐に注記を復活させても検出できない。 + # 5xx 経路を直積に入れることで固定する。 + assert file_stream.size == 6 + arrange_chunked_commit(provider) + path = WaterButlerPath('/foobah', prepend=provider.prefix) + arrange_commit_failure(provider, transport, error_code) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.QUOTA_EXCEEDED_MESSAGE in exc.value.message + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_quota_507_without_a_body_suppresses_the_notice(self, provider, file_stream, + mock_time): + # ``_is_quota_exhaustion`` は本文のコードに関係なく HTTP 507 を + # クォータ扱いする(ベンダ固有コードを網羅できないための意図的な + # フォールバック)。分類表はこの入力を知らないので、抑止を分類表側に + # 置くと 507 + 注記の矛盾文面が復活する —— 設計レビューの実測で + # クォータ判定 True の 4/5 が表に載らないことを確認している。 + assert file_stream.size == 6 + arrange_chunked_commit(provider) + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider.make_request = MockCoroutine( + side_effect=exceptions.UploadError('no body', code=507)) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert exc.value.code == HTTPStatus.INSUFFICIENT_STORAGE + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE not in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('transport', OBSERVED_TRANSPORTS) + @pytest.mark.parametrize('error_code, expect_notice', [ + # 前後の空白は除去してから照合する。XML の整形で + # ``\n AccessDenied\n`` の形が現れうるため。 + ('\n AccessDenied\n', False), + (' AccessDenied ', False), + ('\tEntityTooSmall ', False), + # 大小文字は畳まない。S3 のエラーコードはベンダ間で大小文字まで + # 一致する識別子なので、畳むと別コードの誤一致を生む。 + ('accessdenied', True), + ('ACCESSDENIED', True), + # 部分一致はしない。前方・後方のどちら向きにも。 + ('AccessDeniedByPolicy', True), + ('XAccessDenied', True), + # 内側の空白は識別子の一部ではないが、除去もしない —— 照合は + # 完全一致なので一致せず UNKNOWN に倒れる(安全側)。 + ('Access Denied', True), + ]) + async def test_commit_notice_follows_the_code_matching_rules( + self, provider, file_stream, mock_time, transport, error_code, expect_notice): + # コード照合の規則。4項目のうち ``None`` は全直積が持っているが、 + # 残る3項目(大小文字を区別する / 前後の空白を除去する / 部分一致 + # しない)は規則としてしか書かれておらず、``_parse_s3_error_body`` の + # ``code.strip()`` を削っても全件緑のまま通ってしまう。 + # + # 規則を「実装の都合」ではなく「設計の要求」として直積で張る。 + # ``.strip()`` が xmltodict の既定挙動と重複していても、その既定に + # 依存していることを固定する意味がある。 + assert file_stream.size == 6 + arrange_chunked_commit(provider) + path = WaterButlerPath('/foobah', prepend=provider.prefix) + arrange_commit_failure(provider, transport, error_code) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload(file_stream, path) + + assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice + + def test_the_xml_parser_is_what_strips_the_code(self, provider): + # ``_parse_s3_error_body`` の ``code.strip()`` は、xmltodict 0.9.0 が + # テキストノードを既定で strip するため現状**冗長**である(実測)。 + # したがって ``.strip()`` を削っても挙動は変わらず、上の照合規則の + # テストでは検出できない —— 等価な変更として受け入れる。 + # + # 受け入れられるのは「誰かが strip している」ことを監視できる場合に + # 限る。依存の更新で xmltodict が strip をやめれば照合の規則は + # ``.strip()`` だけが支えることになるので、その切り替わりをここで + # 検知する。このテストが落ちたら ``.strip()`` は冗長ではなくなる。 + parsed = xmltodict.parse('\n AccessDenied\n') + assert parsed['Error']['Code'] == 'AccessDenied' + + @pytest.mark.parametrize('raw, expected', [ + ('\n QuotaExceeded\n', True), + ('quotaexceeded', False), + ('XQuotaExceededFoo', False), + ]) + def test_quota_detection_follows_the_same_matching_rules(self, provider, raw, expected): + # 照合の規則はクォータ照合にも同じく適用される。分類表と設定値リストで + # 規則が食い違うと、片方だけが空白付きコードを取り違える。 + err = storage_error({'response': commit_error_xml(raw)}, code=400) + assert provider._is_quota_exhaustion(err) is expected + + # 書き換え: ``test_commit_outcome_note_falls_back_to_the_status_class`` + # → ``test_commit_outcome_note_ignores_the_status_class`` + # + # ステータスクラス規則は廃止した。同じ規則を別の名前で検証し続ける + # テストは「廃止した規則を検証しつつ合格しているテスト」そのものに + # なるので、主張を反転させて置き換える。 + @pytest.mark.parametrize('code', [ + # No status at all: the connection dropped. + # + # 507 is deliberately absent: ``_is_quota_exhaustion`` counts it as + # quota exhaustion whatever the body says, so it is the one status the + # notice *does* depend on -- and it is a suppression, not a class rule. + # ``test_quota_507_without_a_body_suppresses_the_notice`` covers it. + None, 500, 503, 403, 400, 499, + # A redirect is a misconfiguration (e.g. the wrong region), not a + # failed commit. + 302, + # ``code`` is not guaranteed to be an int: ``exception_from_response`` + # passes through whatever the caller supplied. + '502', 'boom', + ]) + @pytest.mark.parametrize('error_code,not_committed', [ + ('AccessDenied', True), + ('InternalError', False), + (None, False), + ]) + def test_commit_outcome_note_ignores_the_status_class(self, provider, code, error_code, + not_committed): + # ``_check_for_200_error`` synthesises HTTP 502 for *every* + # 200-with-```` body -- the shape a failed + # CompleteMultipartUpload actually takes -- so the status reads "5xx" + # for responses the storage was perfectly definite about. Any rule + # that consults it is deciding on an artefact of WaterButler's own + # error construction. + err = pd_provider._mark_commit_outcome_unknown(pd_provider._mark_storage_response( + exceptions.UploadError({'response': commit_error_xml(error_code)}, code=code))) + note = provider._commit_outcome_note(err) + assert (note == '') is not_committed + assert (note == provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE) is not not_committed + + def test_the_definitive_rejection_table_is_exactly_the_designed_nine(self): + # 表の**内容**を独立に固定する。下のテストが個々の行を守り、これが + # 「行が増えていないこと」を守る。両方ないと、行を足す変異(過少注記 + # 側=回復に管理者が要る方向)が誰にも見つからない。 + assert pd_provider.DEFINITIVE_REJECTION_CODES == frozenset(DEFINITIVE_REJECTION_CODES) + + @pytest.mark.parametrize('error_code', DEFINITIVE_REJECTION_CODES) + def test_every_definitive_rejection_code_suppresses_the_notice(self, provider, error_code): + # 分類表の 9 コードそれぞれについて、1 コードを削除する変更が検出 + # されること。全直積テストは代表 3 件しか流さないので、残り 6 件を + # 守るテストが無ければ「表に載せたが誰も見ていない行」が生まれる。 + # + # コードは**テスト側に書き写してある**。 + # ``sorted(pd_provider.DEFINITIVE_REJECTION_CODES)`` からパラメータを生成すると、 + # 表から行を消したときパラメータごと消えて検出できない —— + # 実装から生成したパラメータは実装を検証できない。 + err = pd_provider._mark_commit_outcome_unknown(pd_provider._mark_storage_response( + exceptions.UploadError({'response': commit_error_xml(error_code)}, code=400))) + assert provider._commit_outcome_note(err) == '' + + @pytest.mark.parametrize('error_code', [ + # 表から意図して外してあるもの。``NoSuchUpload`` は 1 回目の + # commit が成功した後の再送で返る(実機 MinIO で二重 Complete を実行し + # 確認済み)ので、未コミットの証拠にはならない。 + 'NoSuchUpload', + # 表に無い実在の 4xx。過剰注記になるが、これは意図した方向である + # (表に無い 4xx は UNKNOWN 側へ倒す)。 + 'InvalidRequest', 'BadDigest', 'RequestTimeTooSkewed', 'NoSuchKey', 'TooManyParts', + # 不定コードと未知コード。 + 'InternalError', 'SlowDown', 'ServiceUnavailable', 'RequestTimeout', 'XVendorMystery', + # 大小違い・部分一致は表に載っていない扱い(照合の規則)。 + 'accessdenied', 'ACCESSDENIED', 'XAccessDeniedFoo', 'AccessDeniedExtra', + # コードが読めなかった。 + None, + ]) + def test_codes_outside_the_table_keep_the_notice(self, provider, error_code): + # UNKNOWN 側への倒しを反転する変異を殺す。表に無いものは **すべて** + # 注記あり —— 表を伸ばし忘れたときに過少注記へ倒れないための向き。 + err = pd_provider._mark_commit_outcome_unknown(pd_provider._mark_storage_response( + exceptions.UploadError({'response': commit_error_xml(error_code)}, code=400))) + assert provider._commit_outcome_note(err) == provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + + def test_commit_outcome_note_does_not_read_a_code_off_a_connection_error(self, provider): + # ``_raw_error_body`` は本文が無いとき ``err.message`` に落ちる。 + # aiohttp の接続エラーは自前の message を持つので、ゲートが無いと + # 「届かなかった応答」が分類表に口を利けてしまう。接続が切れた時点で + # 観測できたコードは無い、というのが唯一の正しい読みである。 + err = pd_provider._mark_commit_outcome_unknown( + aiohttp.ServerDisconnectedError(commit_error_xml('AccessDenied'))) + assert pd_provider._is_storage_response(err) is False + assert provider._commit_outcome_note(err) == provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE + + def test_commit_outcome_note_needs_the_mark(self, provider): + # Without the mark there was never a commit in flight, whatever the + # status says. + err = exceptions.UploadError('boom', code=500) + assert provider._commit_outcome_note(err) == '' + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('chunked', [False, True]) + async def test_connection_error_log_does_not_leak_the_signature( + self, provider, file_stream, mock_time, caplog, chunked): + # aiohttp 3.6.2 builds this exact message in ``ClientRequest.write_bytes`` + # when the socket dies mid-body: + # + # new_exc = ClientOSError(exc.errno, + # 'Can not write request body for %s' % self.url) + # + # and ``self.url`` is the presigned URL this provider signs with SigV4. + # A dropped connection during the upload is precisely the event this PR + # exists to handle, so this is the common path, not a corner case: + # logging the exception renders the signature into the log and, through + # the exception chain, into the traceback Sentry keeps. + assert file_stream.size == 6 + provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 if chunked else 4096 + provider.CHUNK_SIZE = 2 + + path = WaterButlerPath('/foobah', prepend=provider.prefix) + presigned = ('https://minio.example/bkt/key?X-Amz-Algorithm=AWS4-HMAC-SHA256' + '&X-Amz-Credential=AKIAEXAMPLE%2F20260913%2Fus-east-1%2Fs3%2Faws4_request' + '&X-Amz-Signature=1f2e3d4c5b6a7988SECRETSIG') + err = aiohttp.ClientOSError(32, + 'Can not write request body for {}'.format(presigned)) + provider.make_request = MockCoroutine(side_effect=err) + + with caplog.at_level(logging.DEBUG, logger=PROVIDER_LOGGER): + with pytest.raises(exceptions.UploadError) as exc: + if chunked: + await provider._chunked_upload(file_stream, path) + else: + await provider._contiguous_upload(file_stream, path) + + logged = '\n'.join(r.getMessage() for r in caplog.records if r.name == PROVIDER_LOGGER) + assert 'X-Amz-Signature' not in logged + assert 'X-Amz-Credential' not in logged + assert 'SECRETSIG' not in logged + # The type is what the log is for, so it still has to be there. + assert 'ClientOSError' in logged + # The chained ``__context__`` renders the original exception -- and its + # message -- into the traceback, so suppressing the log alone is not + # enough. The translation paths already use ``from None``; the + # connection paths have to match. + assert exc.value.__cause__ is None + assert exc.value.__suppress_context__ is True + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('failure, expected_code', [ + ('connection', HTTPStatus.BAD_GATEWAY), + ('unexpected', HTTPStatus.INTERNAL_SERVER_ERROR), + ]) + async def test_commit_failure_does_not_chain_the_presigned_url( + self, provider, file_stream, mock_time, failure, expected_code): + # ``_chunked_upload`` の ``raise ... from None`` は7箇所あるが、 + # **commit で落ちたときに通る2箇所**(``CONNECTION_ERRORS`` 分岐と + # 500 分岐)は、削除しても他のテストが全件緑のまま通ってしまう。 + # + # 既存の + # ``test_connection_error_log_does_not_leak_the_signature`` は + # ``make_request`` そのものを潰すため、``_create_upload_session`` の段階で + # 落ちる —— 守っているのはセッション作成側の ``from None`` であって、 + # commit 側ではない。ここでは commit まで到達させる。 + # + # 漏れるのは presigned SigV4 URL の署名クエリで、それが + # ``__context__`` 経由でトレースバックへ入り、Sentry に保存される。 + # 抑止は挙動を変えないが、等価な変更ではない + # —— ``__suppress_context__`` は観測できる。 + presigned = ('https://minio.example/bkt/key?X-Amz-Algorithm=AWS4-HMAC-SHA256' + '&X-Amz-Credential=AKIAEXAMPLE%2F20260913%2Fus-east-1%2Fs3%2Faws4_request' + '&X-Amz-Signature=1f2e3d4c5b6a7988SECRETSIG') + if failure == 'connection': + err = aiohttp.ClientOSError( + 32, 'Can not write request body for {}'.format(presigned)) + else: + # ``UploadError`` でも ``CONNECTION_ERRORS`` でもない例外 = 500 分岐。 + err = ValueError('unexpected failure while committing to {}'.format(presigned)) + + arrange_chunked_commit(provider) + provider._make_upload_request = MockCoroutine(side_effect=err) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload( + file_stream, WaterButlerPath('/foobah', prepend=provider.prefix)) + + assert exc.value.code == expected_code + assert exc.value.__cause__ is None + assert exc.value.__suppress_context__ is True + assert 'SECRETSIG' not in str(exc.value.message) + # 出口そのものは合っていること(分岐を取り違えたテストにしない)。 + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_create_session_parse_failure_log_omits_repr(self, provider, mock_time, caplog): + # Same rule for the CreateMultipartUpload parse failure: identify the + # error by type, and bound the body with the shared constant rather + # than a second hard-coded 512. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + body = b'' + b'z' * 4096 + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=body) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with caplog.at_level(logging.DEBUG, logger=PROVIDER_LOGGER): + with pytest.raises(exceptions.UploadError): + await provider._create_upload_session(path) + + logged = '\n'.join(r.getMessage() for r in caplog.records if r.name == PROVIDER_LOGGER) + assert 'ExpatError' in logged + # ``{!r}`` on the exception is what pulls arbitrary upstream text into + # the log; the type name carries the diagnostic value here. + assert 'ExpatError(' not in logged + assert len(logged) < 2 * pd_provider.ERROR_BODY_LOG_LIMIT def test_parse_s3_error_body_non_xml(self, provider): err = storage_error({'response': 'not xml at all'}, code=500) @@ -2470,6 +3403,7 @@ async def test_create_upload_session_invalid_response(self, provider, mock_time) resp = mock.Mock() resp.read = MockCoroutine(return_value=b'this is not the expected xml') + resp.release = MockCoroutine() provider.make_request = MockCoroutine(return_value=resp) with pytest.raises(exceptions.UploadError) as exc: @@ -2499,6 +3433,7 @@ async def test_create_upload_session_blank_upload_id(self, provider, mock_time, resp = mock.Mock() resp.read = MockCoroutine(return_value=body.encode('utf-8')) + resp.release = MockCoroutine() provider.make_request = MockCoroutine(return_value=resp) with pytest.raises(exceptions.UploadError) as exc: @@ -2561,6 +3496,75 @@ async def test_complete_multipart_upload_releases_when_read_fails(self, provider assert resp.release.call_count == 1 + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_complete_multipart_upload_request_failure_is_marked(self, provider, mock_time): + # Three ways a commit can end with its outcome unknown; this is the + # first -- the commit request itself dies mid-flight. The ``except`` + # around it is deliberately ``Exception`` rather than ``UploadError``, + # because a dropped connection is not an ``UploadError``, and that is + # precisely the case where the storage may have received the whole + # request and assembled the object anyway. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider.make_request = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + + with pytest.raises(aiohttp.ServerDisconnectedError) as exc: + await provider._complete_multipart_upload(path, 'EXAMPLEUPLOADID', + [{'ETAG': '"etag1"'}]) + + assert pd_provider._is_commit_outcome_unknown(exc.value) + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_create_upload_session_releases_when_read_fails(self, provider, mock_time): + # Same defect ``_complete_multipart_upload`` had, 300 lines earlier: a + # storage running out of room drops the connection while the body is + # being read, and without a ``finally`` the connection leaks -- on the + # path that is by definition already under pressure. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + + resp = mock.Mock() + resp.read = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(aiohttp.ServerDisconnectedError): + await provider._create_upload_session(path) + + assert resp.release.call_count == 1 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_create_upload_session_releases_on_success(self, provider, mock_time): + path = WaterButlerPath('/foobah', prepend=provider.prefix) + body = ('' + 'EXAMPLEUPLOADID' + '').encode('utf-8') + + resp = mock.Mock() + resp.read = MockCoroutine(return_value=body) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + assert await provider._create_upload_session(path) == 'EXAMPLEUPLOADID' + assert resp.release.call_count == 1 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_list_uploaded_chunks_releases_when_read_fails(self, provider, mock_time): + path = WaterButlerPath('/foobah', prepend=provider.prefix) + + resp = mock.Mock() + resp.status = HTTPStatus.OK + resp.read = MockCoroutine(side_effect=aiohttp.ServerDisconnectedError()) + resp.release = MockCoroutine() + provider.make_request = MockCoroutine(return_value=resp) + + with pytest.raises(aiohttp.ServerDisconnectedError): + await provider._list_uploaded_chunks(path, 'EXAMPLEUPLOADID') + + assert resp.release.call_count == 1 + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_create_upload_session_strips_upload_id(self, provider, mock_time): @@ -2578,6 +3582,7 @@ async def test_create_upload_session_strips_upload_id(self, provider, mock_time) resp = mock.Mock() resp.read = MockCoroutine(return_value=body.encode('utf-8')) + resp.release = MockCoroutine() provider.make_request = MockCoroutine(return_value=resp) assert await provider._create_upload_session(path) == 'EXAMPLEUPLOADID' @@ -2905,6 +3910,156 @@ async def test_chunked_upload_complete_multipart_upload_error(self, provider, assert aiohttpretty.has_call(method='POST', uri=complete_url, params=params) + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('status', [408, 502, 503, 504]) + async def test_complete_multipart_upload_is_sent_exactly_once( + self, provider, upload_parts_headers_list, mock_time, generate_url_helper, status): + # 注記の判定はコード単独で行い、そのコードは「唯一の試行の結果」で + # なければならない。core の既定 ``retry=2`` では、504 等を受けた再送で + # UploadId が消費済みになり、1回目の commit が成功していても2回目は + # ``NoSuchUpload`` が返る —— 観測コードが最後の試行の結果に化けた + # 時点で、分類表は意味を失う。 + # + # 「``retry=0`` と書いてあること」ではなく「**効いていること**」を + # 固定するために、実 HTTP を流して POST の本数を直接数える。 + # 引数を渡す側だけを見るテストでは、書いてあることしか固定できない。 + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + headers_list = json.loads(upload_parts_headers_list).get('headers_list') + headers_list = [{k.upper(): v for k, v in headers.items()} for headers in headers_list] + + payload = '' + for i, part in enumerate(headers_list): + payload += '{}{}'.format( + i + 1, xml.sax.saxutils.escape(part['ETAG'])) + payload += '' + payload = payload.encode('utf-8') + headers = { + 'Content-Length': str(len(payload)), + 'Content-MD5': compute_md5(BytesIO(payload))[1], + 'Content-Type': 'text/xml', + } + + complete_url = generate_url_helper(key=path.full_path, method='POST', expires=200, + headers=headers, query_parameters=params) + error_body = ('' + 'SlowDown' + 'Please reduce your request rate.') + aiohttpretty.register_uri('POST', complete_url, status=status, + body=error_body.encode('utf-8')) + + with pytest.raises(exceptions.UploadError): + await provider._complete_multipart_upload(path, upload_id, headers_list) + + # 再送対象そのものを固定する。core 側が ``retry_on`` を広げたときに、 + # このパラメータ集合が監視として不足したことを知らせるため。 + assert provider._retry_on == {408, 502, 503, 504} + assert status in provider._retry_on + assert len(aiohttpretty.calls) == 1 + + @pytest.mark.asyncio + @pytest.mark.parametrize('redirect_status', [307, 308]) + async def test_complete_multipart_upload_does_not_follow_a_redirect( + self, provider, upload_parts_headers_list, mock_time, redirect_status): + # ``retry=0`` は core の ``make_request`` の再送ループしか止めない。 + # 307/308 は「メソッドと本文を保って再送せよ」という指示で、その追随は + # aiohttp 自身が ``allow_redirects`` の既定値 ``True`` で行う —— つまり + # core の再送予算をまったく使わずに commit の POST が2本出る。 + # 2本目が消費済み UploadId や別ホスト向けの署名で弾かれると、 + # 分類表が読む「観測コード」は1本目(実際の commit)ではなく2本目の + # 結果に化ける。1本目が成功していた場合、注記なしの確定拒否コードで + # 「何も保存されていない」と断言してしまう。 + # + # ``aiohttpretty`` では固定できない。リダイレクト追随は aiohttp の + # ``ClientSession._request`` 内のループで起きるが、aiohttpretty は + # その *上* で応答を差し込むため、追随そのものが再現されない。 + # 実サーバを立ててハンドラの呼び出し回数を数えるしかない。 + calls = [] + + async def first(request): + await request.read() + calls.append(request.path) + raise web.HTTPTemporaryRedirect(location='/second') \ + if redirect_status == 307 else web.HTTPPermanentRedirect(location='/second') + + async def second(request): + # 追随してしまった2本目。実storageなら消費済み UploadId で + # ``NoSuchUpload``、別ホストなら署名不一致になる。ここでは + # 「注記なし」に落ちる確定拒否コードを返し、追随が起きた場合に + # 危険側へ倒れることを明示する。 + await request.read() + calls.append(request.path) + return web.Response( + status=403, content_type='application/xml', + text='' + 'SignatureDoesNotMatch' + 'The request signature we calculated does not match.' + '') + + app = web.Application() + app.router.add_post('/first', first) + app.router.add_post('/second', second) + + async with commit_server(provider, app) as server: + path = WaterButlerPath('/foobah', prepend=provider.prefix) + headers_list = json.loads(upload_parts_headers_list).get('headers_list') + headers_list = [{k.upper(): v for k, v in headers.items()} + for headers in headers_list] + + with mock.patch.object(provider.connection, 'generate_presigned_url', + return_value=server.url): + with pytest.raises(exceptions.UploadError): + await provider._complete_multipart_upload( + path, 'EXAMPLEUPLOADID', headers_list) + + # commit の POST はちょうど1本。2本目が出ていれば ``/second`` が + # 記録されるので、失敗時に「どこまで行ったか」が読める。 + assert calls == ['/first'] + + @pytest.mark.asyncio + async def test_commit_answer_that_cannot_be_decoded_still_notices( + self, provider, file_stream, mock_time): + # モック注入の 3 セルは「注入点が正しい」ことを前提にしている。 + # この1本は前提ごと固定する —— ストレージが返した本文が UTF-8 として + # 読めないとき、core の ``exception_from_response`` は decode の途中で + # ``UnicodeDecodeError`` を出す。これは ``UploadError`` でも + # ``CONNECTION_ERRORS`` でもないので 500 分岐に落ちるが、commit は + # 既にソケットへ出ている = 組み立てが起きたかどうかは分からない。 + # どの層でこの例外が生まれるかが core の変更で動いても、注記の有無は + # 動いてはならない。 + # + # リダイレクトは関係しない。307 はこの出口への到達手段のひとつに + # すぎず、素の 403 でも同じ経路を通る。 + calls = [] + + async def first(request): + await request.read() + calls.append(request.path) + # 宣言は XML だが中身は UTF-8 として不正なバイト列。 + return web.Response(status=403, content_type='application/xml', + body=b'\xff\xfeAccessDenied') + + app = web.Application() + app.router.add_post('/first', first) + + async with commit_server(provider, app) as server: + arrange_chunked_commit(provider) + with mock.patch.object(provider.connection, 'generate_presigned_url', + return_value=server.url): + with pytest.raises(exceptions.UploadError) as exc: + await provider._chunked_upload( + file_stream, WaterButlerPath('/foobah', prepend=provider.prefix)) + + assert calls == ['/first'] + # ストレージ由来と断定できないので 500。本文のコードは読めていないので + # 分類表は関与せず、UNKNOWN に倒れて注記が付く。 + assert exc.value.code == HTTPStatus.INTERNAL_SERVER_ERROR + assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message + # 読めなかった本文の中身が、そのまま利用者へ出てはいけない。 + assert 'AccessDenied' not in exc.value.message + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_abort_chunked_upload_session_deleted(self, provider, generic_http_404_resp, @@ -2985,6 +4140,155 @@ async def test_abort_no_such_upload_with_parts_left_still_warns( assert aborted is False + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_confirmation_is_fail_closed(self, provider, mock_time): + # Decision (B) rests entirely on this: the confirmation exists because + # ``NoSuchUpload`` on the DELETE does not establish that the *parts* + # are gone, and resolving an unanswerable question in favour of + # "already clean" would suppress the "remove them manually" warning in + # exactly the case it is needed. Nothing was holding that line. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._list_uploaded_chunks = MockCoroutine(side_effect=Exception('boom')) + + assert await provider._abort_confirmed_by_list_parts(path, 'EXAMPLEUPLOADID') is False + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_confirmation_failure_still_warns(self, provider, mock_time, + generate_url_helper): + # The same thing through the public entry point: an unconfirmable abort + # has to report failure so the caller appends the manual-cleanup notice. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + abort_url = generate_url_helper(key=path.full_path, method='DELETE', expires=100, + headers={}, query_parameters=params) + no_such_upload = ('' + 'NoSuchUpload' + 'The specified upload does not exist.') + aiohttpretty.register_uri('DELETE', abort_url, body=no_such_upload.encode('utf-8'), + status=404) + provider._list_uploaded_chunks = MockCoroutine(side_effect=Exception('boom')) + + assert await provider._abort_chunked_upload(path, upload_id) is False + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_confirmation_is_a_single_round_trip(self, provider, mock_time, + generate_url_helper): + # Decision (A), 2026-09-13: the confirmation buys safety for the cost of + # one round trip, and that is the whole bargain. Nested inside the + # abort retry loop *and* inside make_request's own retry budget it was + # worth up to 2 x 3 = 6 ListParts requests against a storage that is + # already struggling. It runs once, on the first iteration. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + abort_url = generate_url_helper(key=path.full_path, method='DELETE', expires=100, + headers={}, query_parameters=params) + list_url = generate_url_helper(key=path.full_path, method='GET', expires=100, + headers={}, query_parameters=params) + no_such_upload = ('' + 'NoSuchUpload' + 'The specified upload does not exist.') + parts_left = ('' + '1' + '"etag1"') + aiohttpretty.register_uri('DELETE', abort_url, body=no_such_upload.encode('utf-8'), + status=404) + aiohttpretty.register_uri('GET', list_url, body=parts_left.encode('utf-8'), status=200) + + assert await provider._abort_chunked_upload(path, upload_id) is False + assert pd_settings.CHUNKED_UPLOAD_MAX_ABORT_RETRIES == 2 + # Second iteration re-sends the DELETE but does not re-ask ListParts. + assert [c['method'] for c in aiohttpretty.calls] == ['DELETE', 'GET', 'DELETE'] + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_abort_confirmation_does_not_use_the_retry_budget(self, provider, mock_time): + # The other half of "one round trip": make_request retries 408/502/503/504 + # twice by default, with a 2s then 4s sleep. A confirmation that cannot + # be obtained has to fall through to the caller's own retry, not stall + # the upload response for six seconds inside a fail-closed check. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + provider._list_uploaded_chunks = MockCoroutine(side_effect=Exception('boom')) + + await provider._abort_confirmed_by_list_parts(path, 'EXAMPLEUPLOADID') + + assert provider._list_uploaded_chunks.call_args[1]['retry'] == 0 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + @pytest.mark.parametrize('status', [408, 502, 503, 504]) + async def test_abort_confirmation_is_sent_exactly_once( + self, provider, mock_time, generate_url_helper, status): + # 上のテストは + # ``_abort_confirmed_by_list_parts`` が ``retry=0`` を *渡している* ことしか + # 見ていない。その引数を受け取る ``_list_uploaded_chunks`` が + # ``**request_kwargs`` を ``make_request`` に流していなければ、渡した + # ``retry=0`` は途中で捨てられ core の既定 ``retry=2`` が効く —— 実際、 + # その ``**request_kwargs`` を削っても全件緑のまま通ってしまう。 + # + # 「伝播していること」ではなく「効いていること」を、実 core 経路で + # GET の本数を数えて固定する。 + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + list_url = generate_url_helper(key=path.full_path, method='GET', expires=100, + headers={}, query_parameters=params) + error_xml = ('' + 'SlowDown' + 'Please reduce your request rate.') + aiohttpretty.register_uri('GET', list_url, body=error_xml.encode('utf-8'), status=status) + + try: + # 確認が取れない = 主張は未確立のまま。呼び出し側の retry に落とす。 + assert await provider._abort_confirmed_by_list_parts(path, upload_id) is False + finally: + for session in provider.session_list: + await session.close() + + assert status in provider._retry_on + assert len(aiohttpretty.calls) == 1 + + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_list_parts_errors_carry_the_storage_tag(self, provider, mock_time, + generate_url_helper): + # Same rule as the abort DELETE: a body may only be read as a storage + # response when the tag says so. ListParts is the request the abort + # confirmation depends on, and an untagged error there degrades quietly + # -- ``_translate_upload_error`` would stop turning it into a 507. + path = WaterButlerPath('/foobah', prepend=provider.prefix) + upload_id = 'EXAMPLEUPLOADID' + params = {'uploadId': upload_id} + list_url = generate_url_helper(key=path.full_path, method='GET', expires=100, + headers={}, query_parameters=params) + error_xml = ('' + 'AccessDeniednope') + aiohttpretty.register_uri('GET', list_url, body=error_xml.encode('utf-8'), status=403) + + with pytest.raises(exceptions.UploadError) as exc: + await provider._list_uploaded_chunks(path, upload_id) + + assert pd_provider._is_storage_response(exc.value) + + @pytest.mark.parametrize('body,expected', [ + ('', True), + ('false', True), + # A truncated listing with no ``Part`` element in *this* page says + # nothing about the pages after it. Reading it as "zero parts" is a + # false positive on the side that suppresses the manual-cleanup + # warning, so the user never learns that billable parts remain. + ('true', False), + ('true' + '1', False), + ]) + def test_no_parts_left_respects_is_truncated(self, provider, body, expected): + assert provider._no_parts_left( + ('' + body).encode('utf-8')) is expected + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_abort_path_errors_carry_the_storage_tag( diff --git a/waterbutler/providers/s3compatsigv4/provider.py b/waterbutler/providers/s3compatsigv4/provider.py index 4c1fe86b7..5a68c7fb7 100644 --- a/waterbutler/providers/s3compatsigv4/provider.py +++ b/waterbutler/providers/s3compatsigv4/provider.py @@ -42,13 +42,80 @@ # and then goes silent. CONNECTION_ERRORS = (aiohttp.ClientError, asyncio.TimeoutError) -# Upper bound on how much of the storage's raw error body is written to the log. -# The body is the only place ``RequestId`` / ``Resource`` survive once the error -# has been translated into a summary message, but it must not be unbounded: a -# misconfigured proxy can answer with a full HTML page. +# Error codes that mean the storage declined the CompleteMultipartUpload +# *before* assembling anything, so the object cannot exist. Everything else -- +# including codes that are not in this table at all, and responses whose code +# could not be observed -- is treated as UNKNOWN and gets the "it may have +# completed" notice. +# +# The fail-safe points this way on purpose. An unnecessary notice sends the +# user to check a file list that turns out not to contain the file: annoying, +# and they recover on their own. A missing notice sends them to upload the +# file a second time when the first one did land, which consumes the quota +# twice and needs an administrator to undo. A table that is extended by hand +# will eventually be out of date, and it has to be out of date in the direction +# that stays recoverable. +# +# Deliberately hardcoded rather than read from ``settings``: the quota codes in +# ``settings.QUOTA_EXCEEDED_ERROR_CODES`` *are* operator-extensible, and the +# suppression rule in ``_commit_outcome_note`` depends on the quota branch +# being evaluated first precisely because the two lists cannot be kept in step. +# Making this one configurable too would reintroduce that coupling. +# +# Only ``EntityTooSmall`` has been confirmed against a real storage (MinIO +# returned it for CompleteMultipartUpload and the object was absent +# afterwards); the other eight rest on the S3 specification and MinIO's own +# error definitions. +DEFINITIVE_REJECTION_CODES = frozenset({ + 'AccessDenied', # no permission, so the commit never started + 'InvalidPart', # the part set does not add up; nothing to assemble + 'InvalidPartOrder', # likewise, out of order + 'EntityTooSmall', # a non-final part is under the minimum + 'EntityTooLarge', # over the size limit; the storage refused it + 'MalformedXML', # the commit body was unreadable + 'SignatureDoesNotMatch', # rejected at signature verification + 'InvalidAccessKeyId', # likewise, at authentication + 'NoSuchBucket', # there is nowhere for the object to exist +}) + +# Upper bound, **in bytes**, on how much of the storage's raw error body is +# written to the log. The body is the only place ``RequestId`` / ``Resource`` +# survive once the error has been translated into a summary message, but it must +# not be unbounded: a misconfigured proxy can answer with a full HTML page. ERROR_BODY_LOG_LIMIT = 512 +def _bounded_body(body): + """The leading :data:`ERROR_BODY_LOG_LIMIT` **bytes** of ``body``, as text. + + Both log sites go through this so that the bound means the same thing at + each. ``'%.*s'`` counts *characters*, which lets a Japanese error message + or a non-ASCII HTML error page through at up to three times the declared + size -- the very case the constant's comment is about. + + Bodies arrive as ``bytes`` straight off the wire in one place and as ``str`` + (already decoded by ``exception_from_response``) in the other, so both are + accepted. Cutting bytes can split a multibyte character, hence ``replace``. + + Cutting the input to the limit is not enough on its own, as measured: + ``replace`` substitutes U+FFFD for every byte it cannot decode, and U+FFFD + is three bytes of UTF-8, so ``b'\\xff' * 512`` came back as 1536 bytes -- + three times the declared bound. The result is + therefore **re-measured** after decoding and cut again, on a character + boundary (``ignore`` drops the partial character the second cut leaves). + The first cut still happens, so a multi-megabyte HTML page is never decoded + in full; it is safe because decoding never shrinks a byte string -- valid + UTF-8 round-trips and invalid bytes expand -- so a 512-byte prefix always + has at least 512 bytes of output to give. + """ + if body is None: + return None + if isinstance(body, str): + body = body.encode('utf-8', 'replace') + head = body[:ERROR_BODY_LOG_LIMIT].decode('utf-8', 'replace') + return head.encode('utf-8')[:ERROR_BODY_LOG_LIMIT].decode('utf-8', 'ignore') + + # Sentinel for "the key is not in the mapping at all". ``xmltodict`` maps an # empty element to ``None`` (````, ```` and # `` `` all become ``{'Error': None}``), so ``None`` on its own @@ -372,6 +439,10 @@ def _check_for_200_error( # attributed upstream: HTTP 502 rather than 500. # ``_translate_upload_error`` refines this to 507 when the body turns # out to be a quota rejection. + # + # The synthesised status carries no information about the outcome, and + # it is not asked to: ``_commit_outcome_note`` classifies on the error + # code alone, and the code is still in ``body`` for it to read. raise _mark_storage_response( exception_type({'response': body}, code=HTTPStatus.BAD_GATEWAY)) @@ -510,6 +581,16 @@ def _parse_s3_error_body(cls, err): # An empty ```` is ``None`` and ```` is a # dict; without a code there is nothing to translate. return None, None + # The code-matching rule requires surrounding whitespace to be + # removed before the code is matched. ``xmltodict`` 0.9.0 already + # strips text nodes, so these ``.strip()`` calls are redundant *today* + # and deleting them changes no behaviour; they are kept rather than + # removed because the rule must not depend silently on a third party's + # default. + # ``test_the_xml_parser_is_what_strips_the_code`` watches that default, + # so a dependency bump that drops it turns these back into the only + # thing holding the rule up instead of quietly breaking the + # classification. return code.strip(), message.strip() if isinstance(message, str) else None @classmethod @@ -532,24 +613,84 @@ def _is_quota_exhaustion(cls, err): error_code = cls._parse_s3_error_body(err)[0] return error_code is not None and error_code in settings.QUOTA_EXCEEDED_ERROR_CODES + @classmethod + def _observed_error_code(cls, err): + """The S3 error code WaterButler actually *saw*, or ``None``. + + The gate on ``_is_storage_response`` is the point of this helper. + ``_raw_error_body`` falls back to ``err.message`` when there is no + response payload, and aiohttp's connection errors carry a message of + their own -- so without the gate, an exception whose message happened to + contain S3-looking XML would be classified as if the storage had + answered. A dropped connection is exactly the case where nothing was + observed, and it must not be able to speak for the storage. + """ + if not _is_storage_response(err): + return None + # ``_parse_s3_error_body`` already strips surrounding whitespace, which + # covers ``\n AccessDenied\n``. Case is *not* folded and + # the comparison below is exact: S3 error codes are identifiers that + # agree between vendors down to the case, and a substring match would + # let ``XQuotaExceededFoo`` pass for ``QuotaExceeded``. + return cls._parse_s3_error_body(err)[0] + + @classmethod + def _commit_outcome(cls, error_code): + """Whether ``error_code`` proves the commit did not happen. + + ``None`` -- no code, or none that could be read -- is UNKNOWN, as is any + code outside :data:`DEFINITIVE_REJECTION_CODES`. + + The two outcomes are named ``NOT_COMMITTED`` and ``UNKNOWN``. The + ``bool`` here is those two names spelled ``True`` and ``False``: + ``True`` is NOT_COMMITTED, ``False`` is UNKNOWN. Kept as a ``bool`` + because the only caller uses it as a condition, and a string would have + to be compared against a constant that a typo could silently defeat. + """ + return error_code is not None and error_code in DEFINITIVE_REJECTION_CODES + @classmethod def _commit_outcome_note(cls, err): """The notice to append when the commit's outcome is genuinely unknown. - Decision: a 4xx is the storage *stating* that it refused the request, so - nothing was assembled and telling the user otherwise would send them - hunting for a file that does not exist. Anything else -- a 5xx, or no - status at all because the connection dropped -- leaves "accepted, then - failed while processing" open, and a silent success there is exactly - what produces a duplicate upload. - - The rule lives here rather than at each mark site so that the three - ways a commit can fail cannot drift apart. + The decision is made from the storage's error code alone: + + 1. **Quota exhaustion suppresses the notice, and is checked first.** + Running out of space is the storage refusing to keep the object, so + "it may have completed" would contradict the very message it is + appended to. This is not a fallback to the status class -- it is a + suppression, and it has to come first because the two lists cannot be + kept in step: ``_is_quota_exhaustion`` counts *any* HTTP 507 whatever + its code, and operators can extend + ``settings.QUOTA_EXCEEDED_ERROR_CODES`` at will, so quota responses + routinely carry codes that no hardcoded table knows. Measured during + the design review: 4 of 5 quota-positive inputs were absent from the + table, and every one of them produced the self-contradictory message. + 2. **Otherwise the code decides**, via + :data:`DEFINITIVE_REJECTION_CODES`. Anything else is UNKNOWN. + + The HTTP status class is deliberately *not* consulted. It cannot carry + this: ``_check_for_200_error`` synthesises HTTP 502 for every + 200-with-```` body, which is the shape a failed + CompleteMultipartUpload actually takes, so the status says "5xx" for + responses the storage was quite definite about. The rule lives here + rather than at each mark site so that the ways a commit can fail cannot + drift apart. + + This is sound only because the commit is sent exactly once. Two things + hold that up, and they stop different re-sends: ``retry=0`` stops + WaterButler's own retry loop, ``allow_redirects=False`` stops the HTTP + client following a 307/308. Both are in + ``_complete_multipart_upload``. Under either kind of re-send the + observed code is the *last* attempt's, and a first attempt that + succeeded would come back as ``NoSuchUpload`` -- at which point + classifying by code says nothing about the upload. """ if not _is_commit_outcome_unknown(err): return '' - code = getattr(err, 'code', None) - if isinstance(code, int) and HTTPStatus.BAD_REQUEST <= code < HTTPStatus.INTERNAL_SERVER_ERROR: + if cls._is_quota_exhaustion(err): + return '' + if cls._commit_outcome(cls._observed_error_code(err)): return '' return cls.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE @@ -580,11 +721,15 @@ def _translate_upload_error(self, err, extra_message=''): # have an administrator remove a stale multipart session. # # This branch is also where an upload call site that forgot - # ``_make_upload_request`` would land, and it degrades *quietly*: - # quota errors would simply stop becoming 507s, with nothing - # failing. DEBUG rather than WARNING because the legitimate case is - # the common one -- this is a breadcrumb for whoever is asking why a - # 507 did not happen, not an alert. + # ``_make_upload_request`` would land, and it degrades *quietly* in + # two ways at once. Quota errors would stop becoming 507s -- and, + # less visibly, ``_commit_outcome_note`` gates on the same tag, so + # the commit notice would stop appearing too: an untagged commit + # failure tells the user nothing was stored when nobody knows that. + # Neither degradation fails anything. DEBUG rather than WARNING + # because the legitimate case is the common one -- this is a + # breadcrumb for whoever is asking why a 507 or a notice did not + # happen, not an alert. logger.debug('Passing through an untagged upload error unmodified: ' 'type=%s status=%s', type(err).__name__, err.code) if not extra_message: @@ -615,15 +760,26 @@ def _translate_upload_error(self, err, extra_message=''): # 3.11 ``'%s' % HTTPStatus.FORBIDDEN`` is ``'HTTPStatus.FORBIDDEN'``. status = int(err.code) if isinstance(err.code, int) else err.code log = logger.warning if is_quota_error else logger.error - log('Storage rejected the upload: status=%s code=%s body=%.*s', - status, error_code, ERROR_BODY_LOG_LIMIT, raw_body) + log('Storage rejected the upload: status=%s code=%s body=%s', + status, error_code, _bounded_body(raw_body)) if is_quota_error: code_note = ' (storage error code: {})'.format(error_code) \ if error_code is not None else '' + # ``outcome_note`` is appended here like everywhere else, and is + # always empty on this path -- ``_commit_outcome_note`` suppresses + # it for quota exhaustion, first, before consulting the code table. + # + # Writing it out rather than omitting it is the point. An earlier + # revision suppressed the notice *here* instead, which meant the + # rule was enforced in two places and neither could be tested: a + # mutation that deleted the quota branch from + # ``_commit_outcome_note`` left all tests green, because this + # omission silently covered for it. One suppression, in one place, + # that a test can actually reach. return exceptions.UploadError( - '{}{}{}{}'.format(self.QUOTA_EXCEEDED_MESSAGE, code_note, outcome_note, - extra_message), + '{}{}{}{}'.format(self.QUOTA_EXCEEDED_MESSAGE, code_note, + outcome_note, extra_message), code=HTTPStatus.INSUFFICIENT_STORAGE, # Running out of storage is an expected, user-resolvable failure: # keep it out of Sentry's error level and the 5xx alerting path. @@ -719,9 +875,19 @@ async def _contiguous_upload(self, stream, path): # is still sending the request body (e.g. when the storage-side # quota has been exceeded). Without this handler the raw client # error propagates as an unexplained HTTP 500. - logger.error('Connection error during contiguous upload: {!r}'.format(err)) + # Type only. aiohttp builds ``ClientOSError(errno, 'Can not write + # request body for ')`` in ``ClientRequest.write_bytes``, and + # for this provider that URL is presigned -- its ``str()`` carries + # ``X-Amz-Credential`` and ``X-Amz-Signature``. A socket dying + # mid-body is the event this handler exists for, so rendering the + # exception here would put the signature in the log on the common + # path, not a rare one. + logger.error('Connection error during contiguous upload: error_type=%s', + type(err).__name__) + # ``from None`` for the same reason: a chained ``__context__`` + # renders the original message into the traceback Sentry stores. raise exceptions.UploadError(self.CONNECTION_INTERRUPTED_MESSAGE, - code=HTTPStatus.BAD_GATEWAY) + code=HTTPStatus.BAD_GATEWAY) from None # S3-compatible server automatically validates Content-MD5 # If MD5 doesn't match, server returns 400 UploadError before writing data @@ -742,9 +908,12 @@ async def _chunked_upload(self, stream, path): # the rendered traceback. raise self._translate_upload_error(err) from None except CONNECTION_ERRORS as err: - logger.error('Connection error during multipart session creation: {!r}'.format(err)) + # Type only, and ``from None``: see ``_contiguous_upload``. The + # presigned URL reaches this path exactly the same way. + logger.error('Connection error during multipart session creation: error_type=%s', + type(err).__name__) raise exceptions.UploadError(self.CONNECTION_INTERRUPTED_MESSAGE, - code=HTTPStatus.BAD_GATEWAY) + code=HTTPStatus.BAD_GATEWAY) from None try: # Step 2. Break stream into chunks and upload them one by one @@ -780,16 +949,38 @@ async def _chunked_upload(self, stream, path): raise self._translate_upload_error( err, extra_message=abort_message) from None if isinstance(err, CONNECTION_ERRORS): - # A connection error carries no status, so the notice applies - # whenever the commit was the request that dropped. This is - # the path a read failure at commit time takes. + # The notice applies whenever the commit was the request that + # dropped -- this is the path a read failure at commit time + # takes. It does not depend on the exception carrying a status: + # ``_commit_outcome_note`` consults the S3 error *code* and + # nothing else, so a ``ClientResponseError`` with a ``.code`` of + # 400, 500 or 507 is treated exactly like a bare + # ``ServerDisconnectedError`` (measured). There is + # deliberately no fallback to the HTTP status class. + # + # ``from None``: aiohttp's message for a mid-body disconnect + # embeds the presigned URL, and a chained ``__context__`` would + # carry it into the traceback. raise exceptions.UploadError( '{}{}{}'.format(self.CONNECTION_INTERRUPTED_MESSAGE, self._commit_outcome_note(err), abort_message), code=HTTPStatus.BAD_GATEWAY, - ) + ) from None + # No ``code=``, so this one defaults to 500 -- not to 502. The + # ``UploadError`` branch above goes through + # ``_translate_upload_error``, which keeps the storage's own status + # (403 stays 403) and answers 507 for quota exhaustion; only the + # ``CONNECTION_ERRORS`` branch is a fixed 502. + # The 500 is still deliberate, and for a reason that does not depend + # on what the others return: an exception that is neither an + # ``UploadError`` (the storage answered) nor a ``CONNECTION_ERRORS`` + # member (the link failed) did not come from upstream at all -- it + # is a defect on this side, and any upstream-attributing status + # would blame the storage for it. Pinned by + # ``test_chunked_upload_unexpected_error_is_a_500``. raise exceptions.UploadError( - '{}{}{}'.format(msg, self._commit_outcome_note(err), abort_message)) + '{}{}{}'.format(msg, self._commit_outcome_note(err), + abort_message)) from None async def _create_upload_session(self, path): """This operation initiates a multipart upload and returns an upload ID. This upload ID is @@ -824,7 +1015,19 @@ async def _create_upload_session(self, path): ), throws=exceptions.UploadError, ) - upload_session_metadata = await resp.read() + try: + upload_session_metadata = await resp.read() + finally: + # A storage that is out of room tends to drop the connection + # mid-body, and ``read()`` then raises with the connection still + # held. A read failure here means no ``UploadId`` was ever + # obtained, so ``_chunked_upload`` returns from its + # ``CONNECTION_ERRORS`` handler at step 1 and never aborts anything. + # The reason to release is the ordinary one: the connector is + # shared, and a leak here is paid for by every later request on this + # provider -- the part uploads, the commit, and the abort that a + # *later* failure does reach. + await resp.release() try: session_data = xmltodict.parse(upload_session_metadata, strip_whitespace=False) # Session upload id is the only info we need @@ -844,9 +1047,11 @@ async def _create_upload_session(self, path): # NOTE: at this point a multipart session MAY have been created on # the storage side but its UploadId is unknown, so it cannot be # aborted here. Log enough information for manual clean-up. - logger.error('Failed to parse the CreateMultipartUpload response: key={} ' - 'error={!r} body={!r}'.format(path.full_path, err, - upload_session_metadata[:512])) + # Type only, and the body bounded by the shared constant: a second + # hard-coded limit here would drift away from the translator's. + logger.error('Failed to parse the CreateMultipartUpload response: key=%s ' + 'error_type=%s body=%s', path.full_path, type(err).__name__, + _bounded_body(upload_session_metadata)) raise exceptions.UploadError( 'Failed to create a multipart upload session: the cloud storage returned an ' 'unexpected response. A stale multipart upload session may remain on the ' @@ -861,9 +1066,9 @@ async def _upload_parts(self, stream, path, session_upload_id): parts = [self.CHUNK_SIZE for i in range(0, stream.size // self.CHUNK_SIZE)] if stream.size % self.CHUNK_SIZE: parts.append(stream.size - (len(parts) * self.CHUNK_SIZE)) - logger.debug('Multipart upload segment sizes: {}'.format(parts)) + logger.debug('Multipart upload segment sizes: %s', parts) for chunk_number, chunk_size in enumerate(parts): - logger.debug(' uploading part {} with size {}'.format(chunk_number + 1, chunk_size)) + logger.debug(' uploading part %s with size %s', chunk_number + 1, chunk_size) metadata.append(await self._upload_part(stream, path, session_upload_id, chunk_number + 1, chunk_size)) return metadata @@ -926,15 +1131,22 @@ def _log_abort_failure(err, session_upload_id, s3_error_code=None): """ status = getattr(err, 'code', None) logger.error('An unexpected error has occurred during the aborting a multipart ' - 'upload. upload_id={} error_type={} status={} error_code={}'.format( - session_upload_id, type(err).__name__, - int(status) if isinstance(status, int) else status, s3_error_code)) + 'upload. upload_id=%s error_type=%s status=%s error_code=%s', + session_upload_id, type(err).__name__, + int(status) if isinstance(status, int) else status, s3_error_code) @staticmethod def _no_parts_left(resp_xml): """Whether a LIST PARTS body reports an empty parts list.""" - uploaded_chunks_list = xmltodict.parse(resp_xml, strip_whitespace=False) - return len(uploaded_chunks_list['ListPartsResult'].get('Part', [])) == 0 + # An element with no children parses to ``None``, not to an empty dict. + result = xmltodict.parse(resp_xml, strip_whitespace=False)['ListPartsResult'] or {} + # ``IsTruncated`` means this is one page of a longer listing, so the + # absence of ``Part`` *here* says nothing about the pages after it. + # This answer is what suppresses the "please remove the parts manually" + # warning, so an incomplete listing must not be read as "nothing left". + if str(result.get('IsTruncated', '')).strip().lower() == 'true': + return False + return len(result.get('Part', [])) == 0 async def _abort_confirmed_by_list_parts(self, path, session_upload_id): """Ask LIST PARTS whether an abort that reported ``NoSuchUpload`` took effect. @@ -949,14 +1161,32 @@ async def _abort_confirmed_by_list_parts(self, path, session_upload_id): unanswerable question has to keep the claim unestablished, not resolve it by default. The caller then logs and retries, which is what it would have done without this branch at all. + + ``retry=0`` keeps this to exactly one round trip. ``make_request`` + would otherwise retry 408/502/503/504 twice, sleeping 2s then 4s, so an + unobtainable confirmation would stall the upload's error response for + six seconds -- inside a check that falls through to the caller's own + retry anyway. + + The bare ``except Exception`` below does swallow + ``asyncio.CancelledError``: on Python 3.6 it derives from ``Exception``, + not from ``BaseException``, so this clause and the four other broad + handlers in this module all catch it. That is accepted, not overlooked. + It is harmless here only because ``waterbutler/`` contains no + ``on_connection_close`` handler, no ``.cancel()`` call and no + ``CancelledError`` reference at all, so nothing in the running service + cancels this task. On Python 3.8+ ``CancelledError`` moved under + ``BaseException`` and the clause stops catching it, which is also the + behaviour that would be wanted -- so this needs no code change, only + re-checking if a cancellation path is ever introduced. """ try: - resp_xml, session_deleted = await self._list_uploaded_chunks(path, session_upload_id) + resp_xml, session_deleted = await self._list_uploaded_chunks( + path, session_upload_id, retry=0) return session_deleted or self._no_parts_left(resp_xml) except Exception as err: logger.warning('Could not confirm a NoSuchUpload abort via ListParts. ' - 'upload_id={} error_type={}'.format(session_upload_id, - type(err).__name__)) + 'upload_id=%s error_type=%s', session_upload_id, type(err).__name__) return False async def _abort_chunked_upload(self, path, session_upload_id): @@ -1029,8 +1259,14 @@ async def _abort_chunked_upload(self, path, session_upload_id): # would answer exactly this way. So confirm it with the same # LIST PARTS check the success path uses, and fall through to # log-and-retry when the check disagrees. + # + # Only on the first iteration: the check is worth one round + # trip, and re-asking it on every retry multiplies requests + # against a storage that has already shown it is struggling. A + # first answer of "not confirmed" is not going to be overturned + # by asking the same endpoint again a moment later. s3_error_code = self._parse_s3_error_body(err)[0] - if s3_error_code == 'NoSuchUpload' and \ + if s3_error_code == 'NoSuchUpload' and iteration_count == 0 and \ await self._abort_confirmed_by_list_parts(path, session_upload_id): is_aborted = True break @@ -1041,17 +1277,20 @@ async def _abort_chunked_upload(self, path, session_upload_id): iteration_count += 1 if is_aborted: - logger.debug('Multi-part upload has been successfully aborted: retries={} ' - 'upload_id={}'.format(iteration_count, session_upload_id)) + logger.debug('Multi-part upload has been successfully aborted: retries=%s ' + 'upload_id=%s', iteration_count, session_upload_id) return True - logger.error('Multi-part upload has failed to abort: retries={} ' - 'upload_id={}'.format(iteration_count, session_upload_id)) + logger.error('Multi-part upload has failed to abort: retries=%s ' + 'upload_id=%s', iteration_count, session_upload_id) return False - async def _list_uploaded_chunks(self, path, session_upload_id): + async def _list_uploaded_chunks(self, path, session_upload_id, **request_kwargs): """This operation lists the parts that have been uploaded for a specific multipart upload. + ``request_kwargs`` is passed through to ``make_request`` so a caller can + override its retry budget; without one the core default applies. + Docs: https://docs.aws.amazon.com/AmazonS3/latest/API/mpUploadListParts.html """ @@ -1078,9 +1317,13 @@ async def _list_uploaded_chunks(self, path, session_upload_id): HTTPStatus.NOT_FOUND, ), throws=exceptions.UploadError, + **request_kwargs ) session_deleted = resp.status == HTTPStatus.NOT_FOUND - resp_xml = await resp.read() + try: + resp_xml = await resp.read() + finally: + await resp.release() return resp_xml, session_deleted @@ -1114,10 +1357,11 @@ async def _complete_multipart_upload(self, path, session_upload_id, parts_metada # Every failure from here on is a failure of the commit itself, and the # commit is the one request whose outcome WaterButler cannot infer: # the parts are already stored, so "did the assemble happen?" is - # answered only by the response. Marking at each of the three places - # the commit can fail is what lets ``_translate_upload_error`` tell the - # user before they upload the file a second time. Which marks actually - # produce the notice is decided there, not here. + # answered only by the response. Marking at both of the places the + # commit can fail -- the request itself, and the reading of its answer + # -- is what lets ``_translate_upload_error`` tell the user before they + # upload the file a second time. Which marks actually produce the + # notice is decided there, not here. try: resp = await self._make_upload_request( 'POST', @@ -1135,6 +1379,36 @@ async def _complete_multipart_upload(self, path, session_upload_id, parts_metada HTTPStatus.CREATED, ), throws=exceptions.UploadError, + # ``retry=0`` is a precondition of ``_commit_outcome_note``, not + # a tuning choice. ``make_request`` would otherwise re-send the + # commit twice on 408/502/503/504 -- and a re-sent commit + # consumes the UploadId, so a first attempt that *succeeded* + # comes back from the second as ``NoSuchUpload``. The code the + # notice classifies would then describe the retry rather than + # the upload, and the classification would be wrong in the + # unrecoverable direction: silently telling the user nothing was + # stored when it was. Same reasoning as the abort confirmation + # in ``_abort_confirmed_by_list_parts``. + # + # The cost is that a transient 503 now fails the upload instead + # of being retried. That failure carries the UNKNOWN notice, so + # the user is told to check the file list -- which is better + # than retrying silently and then asserting the wrong outcome. + retry=0, + # ``retry=0`` alone does not deliver that precondition: it stops + # WaterButler's own retry loop, not the HTTP client's. 307 and + # 308 mean "re-send, method and body intact", and aiohttp obeys + # them by default -- a second commit POST that costs no retry + # budget at all. Everything said above about a re-sent commit + # applies verbatim to that one. + # + # Refusing to follow means a storage that legitimately answers + # the commit with a 307 now fails the upload. That failure is + # an unclassifiable status, so it carries the UNKNOWN notice: + # the user is told to check the file list for a file that was in + # fact stored. Over-noticing is inside the tolerance this + # provider accepts; asserting the wrong outcome is not. + allow_redirects=False, ) except Exception as err: _mark_commit_outcome_unknown(err) diff --git a/waterbutler/providers/s3compatsigv4/settings.py b/waterbutler/providers/s3compatsigv4/settings.py index 19b1172b6..9c8e41ac2 100644 --- a/waterbutler/providers/s3compatsigv4/settings.py +++ b/waterbutler/providers/s3compatsigv4/settings.py @@ -24,11 +24,7 @@ # This list is not exhaustive, so ``_translate_upload_error`` additionally # treats HTTP 507 as a quota failure regardless of the error code. # Deployments can replace this list via the provider config when their storage -# vendor uses a different error code. ``get_object`` is required here: plain -# ``get`` returns the raw string when the value comes from an envvar, which -# would silently turn the membership test into substring matching. - - +# vendor uses a different error code. QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS = [ 'QuotaExceeded', 'XMinioAdminBucketQuotaExceeded', @@ -39,6 +35,14 @@ def _read_error_codes(): """Read the configured error-code list, surviving anything the envvar holds. + ``get_object`` is required here rather than plain ``get``: ``get`` returns + the raw string when the value comes from an envvar, i.e. the *undecoded* + JSON text. A bare ``str`` takes ``_normalise_error_codes``'s scalar + branch and becomes ``frozenset({'["QuotaExceeded"]'})``, one entry holding + the JSON text itself. That is worse than substring matching, not better: + no storage error code can ever equal it, so quota detection silently stops + working everywhere except the HTTP 507 fallback. + ``SettingsDict.get_object`` calls ``json.loads`` with no ``try``, so an envvar that is not valid JSON raises while this module is being imported. Normalising ``get_object``'s *return value* -- which is what @@ -93,7 +97,14 @@ def _normalise_error_codes(configured): a result that is indistinguishable from a working configuration until a quota error goes unrecognised in production. There is no reading of ``{"QuotaExceeded": 507}`` that makes the operator's intent unambiguous. + + ``None`` is treated as "not configured". JSON ``null`` decodes to it and + it is neither a ``Mapping``, a ``str``, nor ``Iterable``, so it used to + reach the scalar branch and produce ``frozenset({'None'})`` -- a + configuration under which no storage error code can ever match. """ + if configured is None: + return frozenset(QUOTA_EXCEEDED_ERROR_CODE_DEFAULTS) if isinstance(configured, Mapping): logger.warning('S3COMPAT_PROVIDER_CONFIG_QUOTA_EXCEEDED_ERROR_CODES is a mapping; ' 'expected a list of error codes. Falling back to the built-in ' From c039e244f09c427e8d3ed3b45cf5479de4e178d7 Mon Sep 17 00:00:00 2001 From: Tomonori Date: Sun, 20 Sep 2026 21:54:21 +0900 Subject: [PATCH 7/8] test comments: translate invariant-guards to English, drop narrative notes (repo convention). Comment/docstring-only change, AST-verified identical. Full rationale preserved in PR #98 description and per-commit messages --- .../providers/s3compatsigv4/test_provider.py | 547 ++++++++---------- 1 file changed, 243 insertions(+), 304 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index 8fdd543f4..a8518070e 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -61,14 +61,15 @@ def storage_error(message, code=403, exception_type=exceptions.UploadError): ) from hmac import compare_digest -# --- commit 成否注記の全直積テストの素材 --------------------------- +# --- Material for the commit-notice cartesian product ---------------------- # -# 不変条件は「同じ操作文脈・同じ *観測* コードなら、輸送経路が違っても同じ結果」。 -# 経路ごとにパラメータ集合が重ならないと矛盾が表に出ないので、経路と -# コードの直積を1本で張る。 +# Invariant: for the same operation and the same *observed* code, every +# transport reaches the same verdict. Eight representative codes: 3 +# definitive rejections (no notice), 3 indeterminate, 1 unknown, 1 missing. # -# コードは分類表の代表8種: 確定拒否(注記なし)3件 / 不定3件 / 未知コード / -# コードなし。未知コードとコードなしは **UNKNOWN に倒す**(fail-safe)。 +# An unknown code and a missing code both fall to UNKNOWN. That is the +# fail-safe direction: an over-reported notice costs the user a re-check, +# an under-reported one silently claims nothing was stored. COMMIT_CODE_CASES = [ ('AccessDenied', False), ('InvalidPart', False), @@ -80,9 +81,9 @@ def storage_error(message, code=403, exception_type=exceptions.UploadError): (None, True), ] -# 確定拒否コードの分類表そのもの。実装(``DEFINITIVE_REJECTION_CODES``)からは -# 生成せず、独立に書き下す。実装から生成すると、行を消す変更で -# パラメータごと消えてしまい、その行を守るテストが存在しなくなる。 +# The classification table, written out independently of the implementation's +# ``DEFINITIVE_REJECTION_CODES``. Generating it from the implementation would +# let a deleted row delete its own parameter, leaving that row unguarded. DEFINITIVE_REJECTION_CODES = [ 'AccessDenied', 'InvalidPart', @@ -95,18 +96,18 @@ def storage_error(message, code=403, exception_type=exceptions.UploadError): 'NoSuchBucket', ] -# コードが観測できる経路。3 x 8 = 24 セル。 +# Transports where the code is observable. 3 x 8 = 24 cells. OBSERVED_TRANSPORTS = ['direct_4xx', 'direct_5xx', 'complete_200_error'] -# コードが観測できない経路。2 x 8 = 16 セル。ストレージが何を言うつもりでも -# WaterButler には届かないので、意図したコードに関わらず UNKNOWN になる。 +# Transports where it is not. 2 x 8 = 16 cells: whatever the storage meant +# to say never reaches WaterButler, so the verdict is UNKNOWN regardless. LATENT_TRANSPORTS = ['disconnect', 'broken_xml'] def commit_error_xml(error_code): - """CompleteMultipartUpload に対する S3 のエラー本文。 + """An S3 error body for CompleteMultipartUpload. - ``error_code`` が ``None`` のときは ```` を欠いた本文を返す - —— 解析はできるがコードが無い、という「コードなし」セルの実体である。 + An ``error_code`` of ``None`` yields a body with no ```` element: + the "missing code" cell, parsable but carrying no verdict. """ if error_code is None: return ('' @@ -116,7 +117,7 @@ def commit_error_xml(error_code): def arrange_commit_failure(provider, transport, error_code): - """``_complete_multipart_upload`` だけを ``transport`` の形で失敗させる。""" + """Fail only ``_complete_multipart_upload``, in the shape of ``transport``.""" if transport == 'direct_4xx': provider.make_request = MockCoroutine(side_effect=exceptions.UploadError( {'response': commit_error_xml(error_code)}, code=400)) @@ -124,32 +125,33 @@ def arrange_commit_failure(provider, transport, error_code): provider.make_request = MockCoroutine(side_effect=exceptions.UploadError( {'response': commit_error_xml(error_code)}, code=500)) elif transport == 'complete_200_error': - # S3 は CompleteMultipartUpload の失敗を HTTP 200 + で返す。 + # S3 reports a failed CompleteMultipartUpload as HTTP 200 + . resp = mock.Mock() resp.read = MockCoroutine(return_value=commit_error_xml(error_code).encode('utf-8')) resp.release = MockCoroutine() provider.make_request = MockCoroutine(return_value=resp) elif transport == 'disconnect': - # 応答が届いていないのでコードは観測できない。aiohttp の例外が持つ - # ``message`` を本文と取り違えて拾う実装を殺すため、あえて S3 の - # エラー XML を message に入れる(``_raw_error_body`` は ``message`` - # にフォールバックする)。 + # No response arrived, so no code is observable. The S3 error XML is + # put in the exception's ``message`` on purpose: ``_raw_error_body`` + # falls back to ``message``, so an implementation that reads a code + # from there rather than from a response body has to fail here. provider.make_request = MockCoroutine( side_effect=aiohttp.ServerDisconnectedError(commit_error_xml(error_code))) elif transport == 'broken_xml': - # 本文は届いたが途中で切れている。コード文字列は本文中に存在するが - # 解析できないので観測はできない —— 部分一致で拾ってはならない。 + # The body arrived but is truncated. The code string is present in it + # yet cannot be parsed, so it is not observed -- a substring match must + # never pick it up. truncated = commit_error_xml(error_code)[:-12].encode('utf-8') resp = mock.Mock() resp.read = MockCoroutine(return_value=truncated) resp.release = MockCoroutine() provider.make_request = MockCoroutine(return_value=resp) - else: # pragma: no cover - パラメータの打ち間違いを黙って通さない + else: # pragma: no cover - a mistyped parameter must not pass silently raise AssertionError('unknown transport: {}'.format(transport)) def arrange_chunked_commit(provider): - """commit だけが失敗する ``_chunked_upload`` の下ごしらえ。""" + """Set up ``_chunked_upload`` so that only the commit fails.""" provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 provider.CHUNK_SIZE = 2 provider._create_upload_session = MockCoroutine(return_value='EXAMPLEUPLOADID') @@ -158,28 +160,24 @@ def arrange_chunked_commit(provider): class commit_server: - """commit を1本だけ受ける ``aiohttp.web`` サーバ。 - - ``aiohttpretty`` は ``ClientSession._request`` の *上* で応答を差し込むため、 - その呼び出しの *内側* で起きる **リダイレクト追随** は再現できない。 - それを固定するテストは実ソケットを使うしかない。 - - デコードできない応答本文のほうは ``aiohttpretty`` でも再現できる —— - 本文は素通しされ、core の ``exception_from_response`` が同じ - ``UnicodeDecodeError`` を出す。それでも実ソケットで1本張るのは、モック - 注入の3セルが寄りかかっている「注入点が正しい」という前提ごと、実 - ``ClientResponse`` で固定するためであって、再現不能だからではない。 - - 起動から後始末までを丸ごと引き受ける。``runner.setup()`` から URL の - 組み立てまでを ``finally`` の外に置くと、サーバを起動したあとに準備が - 失敗したとき、待ち受けソケットと provider のセッションが開いたまま - 次のテストへ持ち越される。 - - ``runner.setup()`` 自体は守っていない。aiohttp 3.6.2 の - ``BaseRunner.cleanup()`` は ``self._server is None`` のとき即 return する - ので、setup が落ちた状態で呼んでも何もしない。ここで使う ``Application`` - は ``on_startup`` / ``on_cleanup`` を1つも登録しないため、setup が途中で - 落ちて資源が残る経路そのものが無い。 + """An ``aiohttp.web`` server that accepts a single commit. + + ``aiohttpretty`` injects responses *above* ``ClientSession._request``, so + the redirect following that happens *inside* that call cannot be + reproduced with it, and pinning it needs a real socket. The real socket + also pins, against a real ``ClientResponse``, the premise the three + mock-injection cells lean on -- that the injection point is correct. + + Startup and teardown are owned here. With ``runner.setup()`` through URL + assembly left outside the ``finally``, a failure after the server started + would carry a listening socket and the provider's sessions into the next + test. + + ``runner.setup()`` itself is not guarded. In aiohttp 3.6.2 + ``BaseRunner.cleanup()`` returns immediately while ``self._server is + None``, so calling it after a failed setup does nothing, and the + ``Application`` used here registers no ``on_startup``/``on_cleanup`` -- + there is no path by which a partial setup leaves resources behind. """ def __init__(self, provider, app): @@ -193,14 +191,14 @@ async def __aenter__(self): try: site = web.TCPSite(self.runner, '127.0.0.1', 0) await site.start() - # aiohttp 3.6.2 はバインド済みポートをここでしか公開しない。 - # 将来この私有属性が消えたら AttributeError で落ちる —— 黙って - # skip されるより、テストが壊れたことが分かるほうがよい。 + # aiohttp 3.6.2 exposes the bound port only here. If this private + # attribute disappears the AttributeError is deliberate: a test + # that visibly breaks beats one that quietly skips. sockets = site._server.sockets assert sockets, 'the test server bound no socket' self.url = 'http://127.0.0.1:{}/first'.format(sockets[0].getsockname()[1]) except Exception: - # ``__aenter__`` が投げると ``__aexit__`` は呼ばれない。 + # ``__aexit__`` is not called when ``__aenter__`` raises. await self.runner.cleanup() raise return self @@ -208,17 +206,17 @@ async def __aenter__(self): async def __aexit__(self, *exc_info): first = None try: - # 1本の close が失敗しても、残りのセッションは閉じる。for を - # 素通しにすると、先頭が投げた時点で後続のセッションが開いたまま - # 次のテストへ残る。 + # One failing close must not strand the rest: letting the loop + # raise would leave every later session open and carry it into + # the next test. for session in self.provider.session_list: try: await session.close() except Exception as err: - # 伝えるのは最初の失敗だけ。後続は握り潰さず、閉じきる。 + # Raise the first failure, but finish closing them all. first = first if first is not None else err finally: - # セッションの close が失敗しても、待ち受けソケットは必ず畳む。 + # The listening socket comes down even if a session close fails. await self.runner.cleanup() if first is not None: raise first @@ -1433,31 +1431,17 @@ async def test_chunked_upload_unexpected_error_is_a_500(self, provider, file_str ]) async def test_chunked_upload_500_branch_notices_only_a_commit_failure( self, provider, file_stream, mock_time, fails_at, expect_notice): - # ``_chunked_upload`` の例外処理には出口が3つある。``UploadError`` と - # ``CONNECTION_ERRORS`` の2つは全直積テストが固定しているが、 - # 「どちらでもない例外」の出口(provider.py の 500 分岐)だけは注記の - # 有無を誰も見ていなかった —— この行の ``_commit_outcome_note(err)`` を - # ``''`` に置き換える変異(N19)が全件緑のまま生き残る。 + # The third exit of ``_chunked_upload``: an exception that is neither + # ``UploadError`` nor a connection error lands in the 500 arm. It is + # reachable -- ``asyncio.CancelledError`` derives from ``Exception`` + # but from neither ``aiohttp.ClientError`` nor ``asyncio.TimeoutError``. # - # 空論ではない。Python 3.6 の ``asyncio.CancelledError`` は - # ``Exception`` の派生でありながら ``CONNECTION_ERRORS`` - # (``aiohttp.ClientError`` と ``asyncio.TimeoutError``)のどちらでも - # ないので、リクエスト実行中のキャンセルはちょうどこの分岐に落ちる。 - # commit の応答読み取り中にキャンセルされれば、その commit が通ったか - # どうかは誰にも分からない = 注記が要る状況そのもの。 - # - # コード軸は取らない。この分岐に来る例外は storage の応答ではないので - # 観測コードは常に ``None`` であり、分類表は最初から関与しない。 - # - # **注入点は ``_complete_multipart_upload`` の外側の境界に置く**。 - # commit メソッドそのものを - # マーク済み例外を投げるモックへ置き換えていたが、それでは - # 「マークが立った例外が来たら注記が出る」までしか固定できない。 - # 実際に印を付けているのは commit 側の ``except Exception`` であり、 - # そこを ``except (UploadError,) + CONNECTION_ERRORS`` へ狭める変異 - # (X18)も、マークを ``isinstance`` ガードの内側へ移す変異(X16)も、 - # 旧方式では 360 件すべて緑のまま生き残った。commit の要求側と - # 応答読取側それぞれに例外を注入すれば、印付けは実コードが行う。 + # Injection stays at the *boundaries* of ``_complete_multipart_upload`` + # (the commit request and the commit answer) and never replaces the + # method with a mock raising an already-marked exception. The code + # under test has to be the thing that applies the mark, or narrowing + # its ``except Exception``, or moving the mark inside an ``isinstance`` + # guard, leaves this test green. assert issubclass(asyncio.CancelledError, Exception) assert not issubclass(asyncio.CancelledError, pd_provider.CONNECTION_ERRORS) @@ -1472,7 +1456,7 @@ async def test_chunked_upload_500_branch_notices_only_a_commit_failure( released = [] class _AnswerWeCannotRead: - """commit の応答。本文の読み取りだけが中断する。""" + """A commit answer whose body read is the part that gets cancelled.""" async def read(self): raise asyncio.CancelledError('cancelled while reading the commit answer') @@ -1481,21 +1465,21 @@ async def release(self): released.append(True) if fails_at == 'parts': - # パート送信中の中断。commit はまだ送られていないので、組み立ては - # 起きていないことが構造的に確定する = 注記は出してはいけない。 + # Cancelled during the parts: no commit was sent, so no assembly + # can have started and the notice must stay off. provider._upload_parts = MockCoroutine( side_effect=asyncio.CancelledError('cancelled during the parts')) provider._make_upload_request = MockCoroutine() else: provider._upload_parts = MockCoroutine(return_value=[{'ETAG': '"e"'}]) if fails_at == 'commit-request': - # 要求の発行中に中断。応答が返っていないので、ストレージが - # 組み立てを始めたかどうかは分からない。 + # Cancelled while sending: no answer came back, so whether + # the storage began assembling is unknowable. provider._make_upload_request = MockCoroutine( side_effect=asyncio.CancelledError('cancelled while sending the commit')) else: - # 応答は返ったが本文を読めなかった。commit が通ったかどうかは - # その本文にしか書かれていない。 + # The answer came back but could not be read, and the body + # is the only place the commit's outcome is written. provider._make_upload_request = MockCoroutine( return_value=_AnswerWeCannotRead()) @@ -1505,11 +1489,11 @@ async def release(self): assert exc.value.code == HTTPStatus.INTERNAL_SERVER_ERROR assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice if fails_at == 'parts': - # commit は1バイトも出ていない。 + # Not one byte of the commit went out. provider._make_upload_request.assert_not_called() else: assert provider._make_upload_request.call_count == 1 - # 応答を受け取った経路では、読めなくても接続は返す。 + # Where an answer arrived, the connection is released even unread. assert released == ([True] if fails_at == 'commit-read' else []) @pytest.mark.asyncio @@ -2072,77 +2056,76 @@ def test_translate_upload_error_log_bound_is_in_bytes(self, provider, caplog): assert len(logged.encode('utf-8')) < limit * 2 @pytest.mark.parametrize('body', [ - # 実測 **1536 バイト**(宣言の3倍)。``replace`` は - # デコード不能な 1 バイトごとに U+FFFD を置き、U+FFFD は UTF-8 で - # 3 バイトある。バイト単位で切ってからデコードするだけでは足りない。 + # Decodes to 1536 bytes, three times the declared limit: ``replace`` + # emits one U+FFFD per undecodable byte and U+FFFD is 3 bytes in + # UTF-8. Truncating bytes and then decoding is not enough. b'\xff' * 512, b'\xff' * 4096, - # 有効な多バイト列。切断点が文字の途中に来ると +1〜+2 バイトになる。 + # Valid multi-byte text: a cut inside a character adds 1-2 bytes. '\u3042' * 512, ('\u3042' * 512).encode('utf-8'), - # 不正バイトと有効文字が混ざった、実際のプロキシ応答に近い形。 + # Invalid bytes mixed with valid text, as a real proxy reply looks. b'' + b'\xc3\x28' * 300 + '\u3042'.encode('utf-8') * 100, - # 上限未満はそのまま通ること(切りすぎていないことの対照)。 + # Under the limit the body passes through: the over-trimming control. b'AccessDenied', 'AccessDenied', b'', '', ]) def test_bounded_body_never_exceeds_the_declared_limit(self, body): - # 定数の宣言は「生エラー本文の先頭 ERROR_BODY_LOG_LIMIT **バイト**」。 - # 宣言が守られているかは、戻り値を UTF-8 で**再計量**して初めて分かる。 - # 既存テストはログ行全体を `limit * 2` 未満で見ていたため、3倍に膨らむ - # 入力(不正バイト列)を見逃していた。 + # The constant declares "the first ERROR_BODY_LOG_LIMIT *bytes* of the + # raw error body", so the only way to check the declaration is to weigh + # the return value in UTF-8 again. Bounding the whole log line instead + # misses inputs that inflate threefold on decode. bounded = pd_provider._bounded_body(body) assert len(bounded.encode('utf-8')) <= pd_provider.ERROR_BODY_LOG_LIMIT - # 切りすぎの検出。判定は**デコード後**の大きさで行う。入力バイト数で - # 書くと `b'\xff' * 512`(入力 512 バイト / デコード後 1536 バイト)で - # 上の表明と両立せず、テスト自体が充足不能になる。 + # Over-trimming check, decided on the *decoded* size. Measuring the + # input instead would make ``b'\xff' * 512`` (512 in, 1536 out) + # contradict the assertion above and the test unsatisfiable. source = body if isinstance(body, bytes) else body.encode('utf-8') decoded = source.decode('utf-8', 'replace') if len(decoded.encode('utf-8')) <= pd_provider.ERROR_BODY_LOG_LIMIT: assert bounded == decoded else: - # 上限超過側で大きさしか見ないと、何も残さず ``''`` を返す - # 実装でも通ってしまう。上限を守ることと本文を残すことは別の - # 要求で、``_bounded_body`` の存在理由は後者にある(調査のために - # 先頭を読ませる)。 + # Size alone would also accept an implementation returning ``''``. + # Staying under the limit and keeping the body are two separate + # requirements and ``_bounded_body`` exists for the second one. # - # 「先頭であること」を接頭辞で固定する。何バイトで切れるかは - # 文字幅と不正バイトの位置で変わるので長さは指定しない。 + # "It is the leading part" is pinned as a prefix, not as a length: + # where the cut lands depends on character width and on where the + # invalid bytes sit. assert bounded assert decoded.startswith(bounded) - # 上限の大半を使い切っていること。1文字だけ返す実装を落とす。 - # U+FFFD / 3バイト文字でも 1/3 は必ず埋まる。 + # Most of the budget has to be used, which kills an implementation + # returning a single character: even 3-byte units fill a third. assert len(bounded.encode('utf-8')) > pd_provider.ERROR_BODY_LOG_LIMIT // 3 @pytest.mark.parametrize('body, expected', [ - # 不正バイト列。``replace`` が 1 バイトごとに U+FFFD を置き、U+FFFD は - # UTF-8 で 3 バイトなので、512 バイトに収まるのは 512 // 3 = 170 文字。 + # Invalid bytes: ``replace`` emits one U+FFFD per byte and U+FFFD is + # 3 bytes in UTF-8, so 512 // 3 = 170 characters fit. (b'\xff' * 512, '\ufffd' * 170), - # 入力がいくら長くても、残る量は同じ。 + # However long the input, the same amount survives. (b'\xff' * 4096, '\ufffd' * 170), - # 有効な3バイト文字。端数の 2 バイトは2段目の ``ignore`` が落とす。 + # Valid 3-byte characters; the 2 leftover bytes go in the ``ignore`` pass. ('\u3042' * 512, '\u3042' * 170), (('\u3042' * 512).encode('utf-8'), '\u3042' * 170), ]) def test_bounded_body_keeps_exactly_the_leading_bytes(self, body, expected): - # 上限超過側を「接頭辞であること」と「上限の 1/3 より大きいこと」 - # だけで見ると、下限が 170 バイトなので、多バイト本文だけを 256 - # バイトへ切り詰める実装(= 現行の保持量のおよそ半分)でも - # 全件通ってしまう。 + # "Is a prefix" plus "larger than a third of the limit" bottoms out at + # 170 bytes, which an implementation trimming multi-byte bodies to 256 + # -- about half of what is kept today -- would still satisfy. # - # 代表入力については独立した**完全な期待値**を置く。170 という数は - # 実装から導かず、「U+FFFD は3バイト / 上限は512バイト」という宣言から - # 手で計算したものである。 + # Representative inputs therefore carry a complete expected value. The + # 170 is computed by hand from the declaration (U+FFFD is 3 bytes, the + # limit is 512), not derived from the implementation. assert pd_provider.ERROR_BODY_LOG_LIMIT == 512 assert pd_provider._bounded_body(body) == expected def test_bounded_body_passes_none_through(self): - # 本文が無い応答(``exception_from_response`` が本文なしで作った - # エラー)では ``None`` が来る。``''`` に潰すと、ログ上で - # 「本文が空だった」と「本文が無かった」が区別できなくなる。 + # An error built without a body yields ``None``. Collapsing that to + # ``''`` would make "the body was empty" and "there was no body" + # indistinguishable in the log. assert pd_provider._bounded_body(None) is None def test_translate_upload_error_quota_logged_as_warning(self, provider, caplog): @@ -2716,18 +2699,6 @@ async def test_chunked_upload_complete_read_failure_warns_upload_may_exist( # The connection was still released despite the read blowing up. assert resp.release.called - # 削除: ``test_chunked_upload_complete_notice_follows_status_class`` - # - # 検証対象だった「4xx/5xx のステータスクラス規則」は廃止した。 - # このテストは新しい規則を適用しても 4/4 合格してしまう —— パラメータが - # (403,'AccessDenied') (400,'InvalidPart') (500,'InternalError') - # (503,'SlowDown') と、ステータスクラス分類とコード分類が**たまたま - # 一致する**組み合わせだけで出来ているためである。 - # 廃止済みの規則を検証しつつ緑であるテストは、落ちるテストより危険で、 - # 将来の実装者に「この規則はまだ有効」と誤認させる。よって残さない。 - # 代替は ``test_commit_notice_depends_only_on_the_observed_code`` - # (全直積 24 セル)と ``test_commit_outcome_note_ignores_the_status_class``。 - @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_parts_connection_error_does_not_claim_a_commit( @@ -2820,20 +2791,17 @@ async def test_chunked_upload_complete_200_with_error_quota(self, provider, file @pytest.mark.asyncio @pytest.mark.aiohttpretty @pytest.mark.parametrize('error_code', [ - # 分類表にあるので抑止される。 + # In the classification table, so the notice is suppressed. 'AccessDenied', 'EntityTooLarge', - # 分類表には**無い**が、クォータ分岐が先に立つので抑止される - # 。以前はこれを表に載せていたが、実機 MinIO が - # commit 経路でクォータを強制しないことが実測で判明したため外した。 + # Not in the table, but the quota branch runs first and suppresses it. 'QuotaExceeded', ]) async def test_complete_200_with_error_code_suppresses_the_notice( self, provider, file_stream, mock_time, error_code): - # 主張は「200 経路だから抑止される」ではなく - # 「**コードが分類表にある / クォータ分岐で抑止される**から」に改めた。 - # 旧アサーションは ``_check_for_200_error`` が立てるマーカーを見ており、 - # 経路が抑止していたのかコードが抑止していたのかを区別できなかった。 - # 経路非依存であることは全直積テストが張る。 + # The claim is that the *code* suppresses the notice -- a table hit or + # the quota branch -- not that the 200 transport does. Asserting on + # the marker ``_check_for_200_error`` sets could not tell the two + # apart. Transport independence is covered by the cartesian product. assert file_stream.size == 6 provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 provider.CHUNK_SIZE = 2 @@ -2866,10 +2834,9 @@ async def test_complete_200_with_error_code_suppresses_the_notice( ]) async def test_complete_200_without_an_error_code_still_warns( self, provider, file_stream, mock_time, body): - # 抑止が許されるのは、ストレージが実際に判定を下したときだけ。 - # コードが読めなければ commit の成否は本当に分からないので、注記は - # 残さなければならない(``None`` は UNKNOWN)。注記の有無はステータス - # クラスではなくコード単独で決まる。 + # Suppression is allowed only where the storage actually reached a + # verdict. With no readable code the commit's outcome is genuinely + # unknown, so the notice stays (``None`` is UNKNOWN). assert file_stream.size == 6 provider.CONTIGUOUS_UPLOAD_SIZE_LIMIT = 5 provider.CHUNK_SIZE = 2 @@ -2895,11 +2862,10 @@ async def test_complete_200_without_an_error_code_still_warns( @pytest.mark.parametrize('error_code,expect_notice', COMMIT_CODE_CASES) async def test_commit_notice_depends_only_on_the_observed_code( self, provider, file_stream, mock_time, transport, error_code, expect_notice): - # 全直積の本体 24 セル。 - # - # 「同じ操作文脈・同じ観測コードなら、経路が違っても同じ結果」を張る。 - # 経路ごとに別のパラメータ集合を使っていたために、3周とも矛盾が - # 表に出なかった —— それがこの直積の存在理由である。 + # The 24 observable cells: the same operation and the same observed + # code must give the same verdict on every transport. Per-transport + # parameter sets cannot expose a contradiction between transports, + # which is why the product is taken in one place. assert file_stream.size == 6 arrange_chunked_commit(provider) path = WaterButlerPath('/foobah', prepend=provider.prefix) @@ -2916,26 +2882,22 @@ async def test_commit_notice_depends_only_on_the_observed_code( @pytest.mark.parametrize('latent_code', [code for code, _ in COMMIT_CODE_CASES]) async def test_commit_notice_when_the_code_cannot_be_observed( self, provider, file_stream, mock_time, transport, latent_code, monkeypatch): - # 全直積の残り 16 セル。 - # - # 接続断と破損 XML では ``_parse_s3_error_body`` がコードを返せない。 - # ストレージが ``AccessDenied`` を言うつもりだったかどうかは - # WaterButler には分からないので、**意図したコードに関わらず** - # UNKNOWN(注記あり)に倒れなければならない。 + # The remaining 16 cells. On a disconnect or a broken body + # ``_parse_s3_error_body`` returns no code, so whatever the storage + # meant to say, the verdict has to fall to UNKNOWN. # - # このセルが空振りでないことに注意: 接続断では S3 のエラー XML が - # 例外の ``message`` に、破損 XML ではコード文字列が本文中に、 - # それぞれ *実在する*。コードを本文以外から拾ったり部分一致で拾う - # 実装は、確定拒否 3 コードのセルで注記を落として落ちる。 + # These cells are not vacuous: the code string really is there -- in + # the exception's ``message`` on a disconnect, inside the truncated + # body on broken XML. An implementation reading it from anywhere but + # a parsed body, or by substring, drops the notice and fails here. assert file_stream.size == 6 arrange_chunked_commit(provider) path = WaterButlerPath('/foobah', prepend=provider.prefix) arrange_commit_failure(provider, transport, latent_code) - # 「観測できない」がこのセル群の前提そのものなので、結論(注記あり) - # だけでなく前提も直接見る。注記を無条件に出す実装でも結論の - # アサートは通ってしまうが、``_observed_error_code`` が本文中の - # コード文字列を拾い始めたらここで落ちる。 + # "Not observable" is the premise of these cells, so the premise is + # asserted alongside the conclusion: an implementation emitting the + # notice unconditionally would satisfy the conclusion on its own. observed = [] real_observed_error_code = pd_provider.S3CompatSigV4Provider._observed_error_code @@ -2961,15 +2923,16 @@ def spy(err): 'XMinioStorageFull']) async def test_quota_branch_suppresses_the_notice_on_every_transport( self, provider, file_stream, mock_time, transport, error_code): - # クォータコードは分類表には載せない(実機 MinIO は commit 経路で - # クォータを強制しないことが実測で判明したため、「未コミットの保証」と - # しては使えない)。代わりに**クォータ分岐が分類表より先に立ち**、 - # 注記を抑止する。「容量が足りません」と「完了しているかもしれません」の - # 連結こそが、この PR が直しているバグの本体である。 + # Quota codes are deliberately absent from the classification table: + # MinIO does not enforce quota on the commit path, so they are no + # guarantee that nothing was committed. The quota branch runs ahead + # of the table instead and suppresses the notice -- "you are out of + # space" next to "it may have completed" is the contradiction this + # change exists to remove. # - # 200 経路だけを見ていると、その経路ではマーカー側ですでに注記が - # '' になっているため、クォータ分岐に注記を復活させても検出できない。 - # 5xx 経路を直積に入れることで固定する。 + # The 200 transport alone cannot pin this, because there the marker + # has already emptied the notice; the 5xx transports are what make a + # regression in the quota branch visible. assert file_stream.size == 6 arrange_chunked_commit(provider) path = WaterButlerPath('/foobah', prepend=provider.prefix) @@ -2986,11 +2949,11 @@ async def test_quota_branch_suppresses_the_notice_on_every_transport( @pytest.mark.aiohttpretty async def test_quota_507_without_a_body_suppresses_the_notice(self, provider, file_stream, mock_time): - # ``_is_quota_exhaustion`` は本文のコードに関係なく HTTP 507 を - # クォータ扱いする(ベンダ固有コードを網羅できないための意図的な - # フォールバック)。分類表はこの入力を知らないので、抑止を分類表側に - # 置くと 507 + 注記の矛盾文面が復活する —— 設計レビューの実測で - # クォータ判定 True の 4/5 が表に載らないことを確認している。 + # ``_is_quota_exhaustion`` treats HTTP 507 as quota exhaustion whatever + # the body says -- a deliberate fallback, since vendor-specific codes + # cannot be enumerated. The table knows nothing about this input, so + # moving the suppression into the table brings the "507 plus notice" + # contradiction straight back. assert file_stream.size == 6 arrange_chunked_commit(provider) path = WaterButlerPath('/foobah', prepend=provider.prefix) @@ -3007,32 +2970,29 @@ async def test_quota_507_without_a_body_suppresses_the_notice(self, provider, fi @pytest.mark.aiohttpretty @pytest.mark.parametrize('transport', OBSERVED_TRANSPORTS) @pytest.mark.parametrize('error_code, expect_notice', [ - # 前後の空白は除去してから照合する。XML の整形で - # ``\n AccessDenied\n`` の形が現れうるため。 + # Surrounding whitespace is stripped before matching: pretty-printed + # XML produces ``\n AccessDenied\n``. ('\n AccessDenied\n', False), (' AccessDenied ', False), ('\tEntityTooSmall ', False), - # 大小文字は畳まない。S3 のエラーコードはベンダ間で大小文字まで - # 一致する識別子なので、畳むと別コードの誤一致を生む。 + # Case is not folded: S3 error codes are identifiers that match down + # to the case across vendors, so folding would create false matches. ('accessdenied', True), ('ACCESSDENIED', True), - # 部分一致はしない。前方・後方のどちら向きにも。 + # No substring matching, in either direction. ('AccessDeniedByPolicy', True), ('XAccessDenied', True), - # 内側の空白は識別子の一部ではないが、除去もしない —— 照合は - # 完全一致なので一致せず UNKNOWN に倒れる(安全側)。 + # Inner whitespace is not part of the identifier and is not removed + # either: the match is exact, so this falls to UNKNOWN, the safe side. ('Access Denied', True), ]) async def test_commit_notice_follows_the_code_matching_rules( self, provider, file_stream, mock_time, transport, error_code, expect_notice): - # コード照合の規則。4項目のうち ``None`` は全直積が持っているが、 - # 残る3項目(大小文字を区別する / 前後の空白を除去する / 部分一致 - # しない)は規則としてしか書かれておらず、``_parse_s3_error_body`` の - # ``code.strip()`` を削っても全件緑のまま通ってしまう。 - # - # 規則を「実装の都合」ではなく「設計の要求」として直積で張る。 - # ``.strip()`` が xmltodict の既定挙動と重複していても、その既定に - # 依存していることを固定する意味がある。 + # The code-matching rules: case-sensitive, surrounding whitespace + # stripped, no substring match. They are design requirements, not + # incidental behaviour, so they are pinned as a product even where the + # implementation happens to get them for free -- the point is to pin + # the dependency on that behaviour. assert file_stream.size == 6 arrange_chunked_commit(provider) path = WaterButlerPath('/foobah', prepend=provider.prefix) @@ -3044,15 +3004,12 @@ async def test_commit_notice_follows_the_code_matching_rules( assert (provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message) is expect_notice def test_the_xml_parser_is_what_strips_the_code(self, provider): - # ``_parse_s3_error_body`` の ``code.strip()`` は、xmltodict 0.9.0 が - # テキストノードを既定で strip するため現状**冗長**である(実測)。 - # したがって ``.strip()`` を削っても挙動は変わらず、上の照合規則の - # テストでは検出できない —— 等価な変更として受け入れる。 - # - # 受け入れられるのは「誰かが strip している」ことを監視できる場合に - # 限る。依存の更新で xmltodict が strip をやめれば照合の規則は - # ``.strip()`` だけが支えることになるので、その切り替わりをここで - # 検知する。このテストが落ちたら ``.strip()`` は冗長ではなくなる。 + # xmltodict strips text nodes by default, which makes the ``.strip()`` + # in ``_parse_s3_error_body`` redundant today, and therefore invisible + # to the matching-rule test above. That is only acceptable while + # "somebody strips" stays observable: if a dependency bump stops + # xmltodict from stripping, this test fails and the ``.strip()`` is + # the only thing still holding the rule up. parsed = xmltodict.parse('\n AccessDenied\n') assert parsed['Error']['Code'] == 'AccessDenied' @@ -3062,17 +3019,11 @@ def test_the_xml_parser_is_what_strips_the_code(self, provider): ('XQuotaExceededFoo', False), ]) def test_quota_detection_follows_the_same_matching_rules(self, provider, raw, expected): - # 照合の規則はクォータ照合にも同じく適用される。分類表と設定値リストで - # 規則が食い違うと、片方だけが空白付きコードを取り違える。 + # The same matching rules apply to the quota codes. If the two lists + # diverge, only one of them mishandles a whitespace-padded code. err = storage_error({'response': commit_error_xml(raw)}, code=400) assert provider._is_quota_exhaustion(err) is expected - # 書き換え: ``test_commit_outcome_note_falls_back_to_the_status_class`` - # → ``test_commit_outcome_note_ignores_the_status_class`` - # - # ステータスクラス規則は廃止した。同じ規則を別の名前で検証し続ける - # テストは「廃止した規則を検証しつつ合格しているテスト」そのものに - # なるので、主張を反転させて置き換える。 @pytest.mark.parametrize('code', [ # No status at all: the connection dropped. # @@ -3108,52 +3059,52 @@ def test_commit_outcome_note_ignores_the_status_class(self, provider, code, erro assert (note == provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE) is not not_committed def test_the_definitive_rejection_table_is_exactly_the_designed_nine(self): - # 表の**内容**を独立に固定する。下のテストが個々の行を守り、これが - # 「行が増えていないこと」を守る。両方ないと、行を足す変異(過少注記 - # 側=回復に管理者が要る方向)が誰にも見つからない。 + # Pin the table's contents as a whole. The test below guards each + # row; this one guards against rows being added, which suppresses a + # notice that should have been shown -- the direction that needs an + # administrator to recover. assert pd_provider.DEFINITIVE_REJECTION_CODES == frozenset(DEFINITIVE_REJECTION_CODES) @pytest.mark.parametrize('error_code', DEFINITIVE_REJECTION_CODES) def test_every_definitive_rejection_code_suppresses_the_notice(self, provider, error_code): - # 分類表の 9 コードそれぞれについて、1 コードを削除する変更が検出 - # されること。全直積テストは代表 3 件しか流さないので、残り 6 件を - # 守るテストが無ければ「表に載せたが誰も見ていない行」が生まれる。 + # All nine rows, so that deleting a single code is caught. The + # cartesian product runs only three representatives; without this the + # other six would be rows nobody checks. # - # コードは**テスト側に書き写してある**。 - # ``sorted(pd_provider.DEFINITIVE_REJECTION_CODES)`` からパラメータを生成すると、 - # 表から行を消したときパラメータごと消えて検出できない —— - # 実装から生成したパラメータは実装を検証できない。 + # The parameters come from the copy at the top of this file, not from + # ``pd_provider.DEFINITIVE_REJECTION_CODES``: parameters generated + # from the implementation cannot test the implementation. err = pd_provider._mark_commit_outcome_unknown(pd_provider._mark_storage_response( exceptions.UploadError({'response': commit_error_xml(error_code)}, code=400))) assert provider._commit_outcome_note(err) == '' @pytest.mark.parametrize('error_code', [ - # 表から意図して外してあるもの。``NoSuchUpload`` は 1 回目の - # commit が成功した後の再送で返る(実機 MinIO で二重 Complete を実行し - # 確認済み)ので、未コミットの証拠にはならない。 + # Deliberately left out of the table: ``NoSuchUpload`` also comes back + # from a resend after a *successful* first commit, so it is no + # evidence that nothing was committed. 'NoSuchUpload', - # 表に無い実在の 4xx。過剰注記になるが、これは意図した方向である - # (表に無い 4xx は UNKNOWN 側へ倒す)。 + # Real 4xx codes absent from the table. Over-reporting the notice is + # the intended direction for anything the table does not name. 'InvalidRequest', 'BadDigest', 'RequestTimeTooSkewed', 'NoSuchKey', 'TooManyParts', - # 不定コードと未知コード。 + # Indeterminate and unknown codes. 'InternalError', 'SlowDown', 'ServiceUnavailable', 'RequestTimeout', 'XVendorMystery', - # 大小違い・部分一致は表に載っていない扱い(照合の規則)。 + # Wrong case and substrings count as absent, per the matching rules. 'accessdenied', 'ACCESSDENIED', 'XAccessDeniedFoo', 'AccessDeniedExtra', - # コードが読めなかった。 + # No code could be read. None, ]) def test_codes_outside_the_table_keep_the_notice(self, provider, error_code): - # UNKNOWN 側への倒しを反転する変異を殺す。表に無いものは **すべて** - # 注記あり —— 表を伸ばし忘れたときに過少注記へ倒れないための向き。 + # Everything outside the table keeps the notice, so that forgetting to + # extend the table errs towards over-reporting rather than silence. err = pd_provider._mark_commit_outcome_unknown(pd_provider._mark_storage_response( exceptions.UploadError({'response': commit_error_xml(error_code)}, code=400))) assert provider._commit_outcome_note(err) == provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE def test_commit_outcome_note_does_not_read_a_code_off_a_connection_error(self, provider): - # ``_raw_error_body`` は本文が無いとき ``err.message`` に落ちる。 - # aiohttp の接続エラーは自前の message を持つので、ゲートが無いと - # 「届かなかった応答」が分類表に口を利けてしまう。接続が切れた時点で - # 観測できたコードは無い、というのが唯一の正しい読みである。 + # ``_raw_error_body`` falls back to ``err.message`` when there is no + # body, and aiohttp's connection errors carry a message of their own. + # Without the gate, a response that never arrived would get a say in + # the classification table. A dropped connection observed no code. err = pd_provider._mark_commit_outcome_unknown( aiohttp.ServerDisconnectedError(commit_error_xml('AccessDenied'))) assert pd_provider._is_storage_response(err) is False @@ -3221,20 +3172,16 @@ async def test_connection_error_log_does_not_leak_the_signature( ]) async def test_commit_failure_does_not_chain_the_presigned_url( self, provider, file_stream, mock_time, failure, expected_code): - # ``_chunked_upload`` の ``raise ... from None`` は7箇所あるが、 - # **commit で落ちたときに通る2箇所**(``CONNECTION_ERRORS`` 分岐と - # 500 分岐)は、削除しても他のテストが全件緑のまま通ってしまう。 + # ``_chunked_upload`` has seven ``raise ... from None`` sites. The two + # taken when the *commit* fails -- the ``CONNECTION_ERRORS`` arm and + # the 500 arm -- are guarded nowhere else: + # ``test_connection_error_log_does_not_leak_the_signature`` kills + # ``make_request`` outright and so never gets past + # ``_create_upload_session``. This test reaches the commit. # - # 既存の - # ``test_connection_error_log_does_not_leak_the_signature`` は - # ``make_request`` そのものを潰すため、``_create_upload_session`` の段階で - # 落ちる —— 守っているのはセッション作成側の ``from None`` であって、 - # commit 側ではない。ここでは commit まで到達させる。 - # - # 漏れるのは presigned SigV4 URL の署名クエリで、それが - # ``__context__`` 経由でトレースバックへ入り、Sentry に保存される。 - # 抑止は挙動を変えないが、等価な変更ではない - # —— ``__suppress_context__`` は観測できる。 + # What leaks is the signature query of the presigned SigV4 URL, + # carried into the traceback through ``__context__`` and kept by + # Sentry. Suppressing it is observable: ``__suppress_context__``. presigned = ('https://minio.example/bkt/key?X-Amz-Algorithm=AWS4-HMAC-SHA256' '&X-Amz-Credential=AKIAEXAMPLE%2F20260913%2Fus-east-1%2Fs3%2Faws4_request' '&X-Amz-Signature=1f2e3d4c5b6a7988SECRETSIG') @@ -3242,7 +3189,7 @@ async def test_commit_failure_does_not_chain_the_presigned_url( err = aiohttp.ClientOSError( 32, 'Can not write request body for {}'.format(presigned)) else: - # ``UploadError`` でも ``CONNECTION_ERRORS`` でもない例外 = 500 分岐。 + # Neither ``UploadError`` nor ``CONNECTION_ERRORS``: the 500 arm. err = ValueError('unexpected failure while committing to {}'.format(presigned)) arrange_chunked_commit(provider) @@ -3256,7 +3203,7 @@ async def test_commit_failure_does_not_chain_the_presigned_url( assert exc.value.__cause__ is None assert exc.value.__suppress_context__ is True assert 'SECRETSIG' not in str(exc.value.message) - # 出口そのものは合っていること(分岐を取り違えたテストにしない)。 + # Confirm the arm under test is the one that actually ran. assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message @pytest.mark.asyncio @@ -3915,15 +3862,14 @@ async def test_chunked_upload_complete_multipart_upload_error(self, provider, @pytest.mark.parametrize('status', [408, 502, 503, 504]) async def test_complete_multipart_upload_is_sent_exactly_once( self, provider, upload_parts_headers_list, mock_time, generate_url_helper, status): - # 注記の判定はコード単独で行い、そのコードは「唯一の試行の結果」で - # なければならない。core の既定 ``retry=2`` では、504 等を受けた再送で - # UploadId が消費済みになり、1回目の commit が成功していても2回目は - # ``NoSuchUpload`` が返る —— 観測コードが最後の試行の結果に化けた - # 時点で、分類表は意味を失う。 + # The notice is decided on the code alone, and that code has to be the + # result of the *only* attempt. Under core's default ``retry=2`` a + # resend after a 504 meets a consumed UploadId and gets + # ``NoSuchUpload`` even though the first commit succeeded; once the + # observed code is the last attempt's, the table is meaningless. # - # 「``retry=0`` と書いてあること」ではなく「**効いていること**」を - # 固定するために、実 HTTP を流して POST の本数を直接数える。 - # 引数を渡す側だけを見るテストでは、書いてあることしか固定できない。 + # Counting the POSTs over real HTTP pins that ``retry=0`` takes + # effect. Inspecting the caller only pins that it is written down. path = WaterButlerPath('/foobah', prepend=provider.prefix) upload_id = 'EXAMPLEUPLOADID' params = {'uploadId': upload_id} @@ -3953,8 +3899,8 @@ async def test_complete_multipart_upload_is_sent_exactly_once( with pytest.raises(exceptions.UploadError): await provider._complete_multipart_upload(path, upload_id, headers_list) - # 再送対象そのものを固定する。core 側が ``retry_on`` を広げたときに、 - # このパラメータ集合が監視として不足したことを知らせるため。 + # Pin the retried statuses too, so that widening core's ``retry_on`` + # reports this parameter set as no longer covering it. assert provider._retry_on == {408, 502, 503, 504} assert status in provider._retry_on assert len(aiohttpretty.calls) == 1 @@ -3963,19 +3909,16 @@ async def test_complete_multipart_upload_is_sent_exactly_once( @pytest.mark.parametrize('redirect_status', [307, 308]) async def test_complete_multipart_upload_does_not_follow_a_redirect( self, provider, upload_parts_headers_list, mock_time, redirect_status): - # ``retry=0`` は core の ``make_request`` の再送ループしか止めない。 - # 307/308 は「メソッドと本文を保って再送せよ」という指示で、その追随は - # aiohttp 自身が ``allow_redirects`` の既定値 ``True`` で行う —— つまり - # core の再送予算をまったく使わずに commit の POST が2本出る。 - # 2本目が消費済み UploadId や別ホスト向けの署名で弾かれると、 - # 分類表が読む「観測コード」は1本目(実際の commit)ではなく2本目の - # 結果に化ける。1本目が成功していた場合、注記なしの確定拒否コードで - # 「何も保存されていない」と断言してしまう。 + # ``retry=0`` stops only core's own retry loop in ``make_request``. A + # 307/308 says "resend with the method and body intact", and aiohttp + # follows it itself under the default ``allow_redirects=True``, so two + # commit POSTs go out without spending any of core's retry budget. + # If the second is refused -- consumed UploadId, signature for another + # host -- the observed code becomes the second attempt's, and a + # definitive rejection there would claim nothing was stored even + # though the first commit succeeded. # - # ``aiohttpretty`` では固定できない。リダイレクト追随は aiohttp の - # ``ClientSession._request`` 内のループで起きるが、aiohttpretty は - # その *上* で応答を差し込むため、追随そのものが再現されない。 - # 実サーバを立ててハンドラの呼び出し回数を数えるしかない。 + # ``aiohttpretty`` cannot pin this; see ``commit_server``. calls = [] async def first(request): @@ -3985,10 +3928,10 @@ async def first(request): if redirect_status == 307 else web.HTTPPermanentRedirect(location='/second') async def second(request): - # 追随してしまった2本目。実storageなら消費済み UploadId で - # ``NoSuchUpload``、別ホストなら署名不一致になる。ここでは - # 「注記なし」に落ちる確定拒否コードを返し、追随が起きた場合に - # 危険側へ倒れることを明示する。 + # The second POST, reached only if the redirect were followed. It + # answers with a definitive rejection code -- the "no notice" + # side -- so that following the redirect fails towards the + # dangerous verdict rather than a harmless one. await request.read() calls.append(request.path) return web.Response( @@ -4014,30 +3957,29 @@ async def second(request): await provider._complete_multipart_upload( path, 'EXAMPLEUPLOADID', headers_list) - # commit の POST はちょうど1本。2本目が出ていれば ``/second`` が - # 記録されるので、失敗時に「どこまで行ったか」が読める。 + # Exactly one commit POST. A second one records ``/second``, so a + # failure here shows how far the request got. assert calls == ['/first'] @pytest.mark.asyncio async def test_commit_answer_that_cannot_be_decoded_still_notices( self, provider, file_stream, mock_time): - # モック注入の 3 セルは「注入点が正しい」ことを前提にしている。 - # この1本は前提ごと固定する —— ストレージが返した本文が UTF-8 として - # 読めないとき、core の ``exception_from_response`` は decode の途中で - # ``UnicodeDecodeError`` を出す。これは ``UploadError`` でも - # ``CONNECTION_ERRORS`` でもないので 500 分岐に落ちるが、commit は - # 既にソケットへ出ている = 組み立てが起きたかどうかは分からない。 - # どの層でこの例外が生まれるかが core の変更で動いても、注記の有無は - # 動いてはならない。 + # When the returned body is not valid UTF-8, core's + # ``exception_from_response`` raises ``UnicodeDecodeError`` mid-decode. + # That is neither ``UploadError`` nor a connection error, so it lands + # in the 500 arm -- but the commit is already on the socket, so + # whether assembly started is unknown. Which layer produces the + # exception may move with core; the notice must not. # - # リダイレクトは関係しない。307 はこの出口への到達手段のひとつに - # すぎず、素の 403 でも同じ経路を通る。 + # This runs over a real socket to pin, with a real ``ClientResponse``, + # the injection-point premise the mock-injection cells rely on. + # Redirects are incidental: a plain 403 takes the same path. calls = [] async def first(request): await request.read() calls.append(request.path) - # 宣言は XML だが中身は UTF-8 として不正なバイト列。 + # Declared as XML, but the bytes are not valid UTF-8. return web.Response(status=403, content_type='application/xml', body=b'\xff\xfeAccessDenied') @@ -4053,11 +3995,11 @@ async def first(request): file_stream, WaterButlerPath('/foobah', prepend=provider.prefix)) assert calls == ['/first'] - # ストレージ由来と断定できないので 500。本文のコードは読めていないので - # 分類表は関与せず、UNKNOWN に倒れて注記が付く。 + # Not provably a storage verdict, so 500. The code was never read, so + # the table does not apply and this falls to UNKNOWN with the notice. assert exc.value.code == HTTPStatus.INTERNAL_SERVER_ERROR assert provider.UPLOAD_MAY_HAVE_COMPLETED_MESSAGE in exc.value.message - # 読めなかった本文の中身が、そのまま利用者へ出てはいけない。 + # The unreadable body must not reach the user verbatim. assert 'AccessDenied' not in exc.value.message @pytest.mark.asyncio @@ -4223,15 +4165,11 @@ async def test_abort_confirmation_does_not_use_the_retry_budget(self, provider, @pytest.mark.parametrize('status', [408, 502, 503, 504]) async def test_abort_confirmation_is_sent_exactly_once( self, provider, mock_time, generate_url_helper, status): - # 上のテストは - # ``_abort_confirmed_by_list_parts`` が ``retry=0`` を *渡している* ことしか - # 見ていない。その引数を受け取る ``_list_uploaded_chunks`` が - # ``**request_kwargs`` を ``make_request`` に流していなければ、渡した - # ``retry=0`` は途中で捨てられ core の既定 ``retry=2`` が効く —— 実際、 - # その ``**request_kwargs`` を削っても全件緑のまま通ってしまう。 - # - # 「伝播していること」ではなく「効いていること」を、実 core 経路で - # GET の本数を数えて固定する。 + # The test above shows only that ``_abort_confirmed_by_list_parts`` + # *passes* ``retry=0``. If ``_list_uploaded_chunks`` stopped + # forwarding ``**request_kwargs`` to ``make_request``, the argument + # would be dropped and core's default ``retry=2`` would apply. + # Counting GETs through the real core path pins that it takes effect. path = WaterButlerPath('/foobah', prepend=provider.prefix) upload_id = 'EXAMPLEUPLOADID' params = {'uploadId': upload_id} @@ -4243,7 +4181,8 @@ async def test_abort_confirmation_is_sent_exactly_once( aiohttpretty.register_uri('GET', list_url, body=error_xml.encode('utf-8'), status=status) try: - # 確認が取れない = 主張は未確立のまま。呼び出し側の retry に落とす。 + # No confirmation: the claim stays unestablished, so fall through + # to the caller's own retry. assert await provider._abort_confirmed_by_list_parts(path, upload_id) is False finally: for session in provider.session_list: From af0768c7b0deb5594db71a7684b08b620e70d38c Mon Sep 17 00:00:00 2001 From: Tomonori Date: Wed, 23 Sep 2026 17:43:58 +0900 Subject: [PATCH 8/8] test comments: correct three factual claims in test_provider.py Comment-only change. AST verified identical to the parent commit; pinned suite 368 passed, flake8 clean. - test_user_facing_messages_keep_their_wording: drop the "emptying QUOTA_EXCEEDED_MESSAGE killed zero tests" figure. The only recorded mutation run for that substitution measured 2 failed (round6 M19), and re-running it today also kills 2, so the figure was never corroborated. Replaced with the mechanism it was standing in for, which is verifiable by reading: emptying a constant neutralises the positive-form assertions that name it without failing any of them. - test_commit_failure_does_not_chain_the_presigned_url: "seven raise ... from None sites" was the whole-file total. _chunked_upload itself has five (AST-counted); the two commit-path arms the comment goes on to describe are unchanged. - test_create_upload_session_releases_when_read_fails: replace the hardcoded "300 lines earlier" with a name reference to test_complete_multipart_upload_always_releases, so the pointer cannot drift as the file is edited. --- tests/providers/s3compatsigv4/test_provider.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/providers/s3compatsigv4/test_provider.py b/tests/providers/s3compatsigv4/test_provider.py index a8518070e..864911c2d 100644 --- a/tests/providers/s3compatsigv4/test_provider.py +++ b/tests/providers/s3compatsigv4/test_provider.py @@ -2337,8 +2337,8 @@ def test_user_facing_messages_keep_their_wording(self, provider): # Every other assertion in this file spells the expected text as # ``provider.``, so changing a constant changes the assertion # with it. Setting one to ``''`` makes ``'' in message`` vacuously true - # and deletes the assertion outright -- measured: emptying - # QUOTA_EXCEEDED_MESSAGE killed zero tests. + # and so deletes every positive-form assertion that names it, without + # failing any of them. # # Pinning the wording once, here, is what gives those assertions teeth. # It is deliberately partial (phrases, not the full string) so that @@ -3172,7 +3172,7 @@ async def test_connection_error_log_does_not_leak_the_signature( ]) async def test_commit_failure_does_not_chain_the_presigned_url( self, provider, file_stream, mock_time, failure, expected_code): - # ``_chunked_upload`` has seven ``raise ... from None`` sites. The two + # ``_chunked_upload`` has five ``raise ... from None`` sites. The two # taken when the *commit* fails -- the ``CONNECTION_ERRORS`` arm and # the 500 arm -- are guarded nowhere else: # ``test_connection_error_log_does_not_leak_the_signature`` kills @@ -3464,7 +3464,8 @@ async def test_complete_multipart_upload_request_failure_is_marked(self, provide @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_create_upload_session_releases_when_read_fails(self, provider, mock_time): - # Same defect ``_complete_multipart_upload`` had, 300 lines earlier: a + # Same defect ``_complete_multipart_upload`` had -- see + # ``test_complete_multipart_upload_always_releases`` above: a # storage running out of room drops the connection while the body is # being read, and without a ``finally`` the connection leaks -- on the # path that is by definition already under pressure.