diff --git a/.changes/next-release/s3-cp-exit-code.json b/.changes/next-release/s3-cp-exit-code.json new file mode 100644 index 000000000000..476ed251e390 --- /dev/null +++ b/.changes/next-release/s3-cp-exit-code.json @@ -0,0 +1,7 @@ +[ + { + "type": "bugfix", + "category": "s3", + "description": "Fix s3 cp returning exit code 0 when upload fails with ReadTimeoutError" + } +] diff --git a/awscli/customizations/s3/results.py b/awscli/customizations/s3/results.py index 184f13a2dc92..87aaf4719ce6 100644 --- a/awscli/customizations/s3/results.py +++ b/awscli/customizations/s3/results.py @@ -150,11 +150,17 @@ def _on_failure(self, future, e): def _is_existing_object_skipped(self, future, exception): # --no-overwrite sends IfNoneMatch, which fails if the object exists. + response = getattr(exception, 'response', None) + if not isinstance(response, dict): + return False + error = response.get('Error', {}) + if not isinstance(error, dict): + return False + call_args = getattr(getattr(future, 'meta', None), 'call_args', None) + extra_args = getattr(call_args, 'extra_args', {}) or {} return ( - hasattr(exception, 'response') - and exception.response.get('Error', {}).get('Code') - == 'PreconditionFailed' - and 'IfNoneMatch' in future.meta.call_args.extra_args + error.get('Code') == 'PreconditionFailed' + and 'IfNoneMatch' in extra_args ) diff --git a/tests/functional/s3/test_cp_command.py b/tests/functional/s3/test_cp_command.py index 9fae40a55f80..075dcd1817a6 100644 --- a/tests/functional/s3/test_cp_command.py +++ b/tests/functional/s3/test_cp_command.py @@ -21,6 +21,7 @@ S3RequestType, S3ResponseError, ) +from botocore.exceptions import ReadTimeoutError from botocore.response import StreamingBody from awscli.compat import BytesIO, OrderedDict @@ -32,6 +33,7 @@ mock, skip_if_windows, ) +from tests import BaseResponse from tests.functional.s3 import ( BaseCRTTransferClientTest, BaseS3CLIRunnerTest, @@ -3986,3 +3988,21 @@ def test_s3_express_warn_emits_warning(self): _, stderr, _ = self.run_cmd(cmd, expected_rc=0) assert "warning: Recursive copies/moves" in stderr + + +class TestCpFailureExitCode(BaseS3CLIRunnerTest): + def test_upload_fails_with_read_timeout_error_returns_exit_code_1(self): + class ReadTimeoutResponse(BaseResponse): + def on_http_request_sent(self, request): + raise ReadTimeoutError(endpoint_url='https://s3.amazonaws.com') + + self.cli_runner.env['AWS_MAX_ATTEMPTS'] = '1' + self.session_stubber.add_response(ReadTimeoutResponse()) + filename = os.path.join(self.config_files.rootdir, 'test.txt') + with open(filename, 'w') as f: + f.write('hello') + cmdline = ['s3', 'cp', filename, 's3://bucket/key'] + result = self.cli_runner.run(cmdline) + self.assertEqual(result.rc, 1) + self.assertIn('upload failed', result.stderr) + self.assertIn('Read timeout on endpoint URL', result.stderr) diff --git a/tests/unit/customizations/s3/test_results.py b/tests/unit/customizations/s3/test_results.py index 49eb277300c1..c7f01a33e53e 100644 --- a/tests/unit/customizations/s3/test_results.py +++ b/tests/unit/customizations/s3/test_results.py @@ -13,7 +13,7 @@ import time -from botocore.exceptions import HTTPClientError +from botocore.exceptions import HTTPClientError, ReadTimeoutError from s3transfer.exceptions import CancelledError, FatalError from awscli.compat import StringIO, queue @@ -213,6 +213,45 @@ def test_on_done_precondition_failed_without_if_none_match(self): ), ) + def test_on_done_failure_with_none_response(self): + subscriber = self.get_result_subscriber(DoneResultSubscriber) + read_timeout_error = ReadTimeoutError( + endpoint_url='https://s3.amazonaws.com' + ) + failure_future = self.get_failed_transfer_future(read_timeout_error) + subscriber.on_done(failure_future) + result = self.get_queued_result() + self.assert_result_queue_is_empty() + self.assertEqual( + result, + FailureResult( + transfer_type=self.transfer_type, + src=self.src, + dest=self.dest, + exception=read_timeout_error, + ), + ) + + def test_on_done_failure_with_malformed_error_field(self): + subscriber = self.get_result_subscriber(DoneResultSubscriber) + malformed_error = HTTPClientError( + request=mock.Mock(), + response={'Error': 'not-a-dict'}, + error='not-a-dict', + ) + failure_future = self.get_failed_transfer_future(malformed_error) + subscriber.on_done(failure_future) + result = self.get_queued_result() + self.assert_result_queue_is_empty() + self.assertEqual( + result, + FailureResult( + transfer_type=self.transfer_type, + src=self.src, + dest=self.dest, + exception=malformed_error, + ), + ) def test_on_done_unexpected_cancelled(self): subscriber = self.get_result_subscriber(DoneResultSubscriber)