diff --git a/.changes/next-release/bugfix-crt-24627.json b/.changes/next-release/bugfix-crt-24627.json new file mode 100644 index 000000000000..bd7f9d50bff4 --- /dev/null +++ b/.changes/next-release/bugfix-crt-24627.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "crt", + "description": "Return error when final rename task fails on downloads" +} diff --git a/awscli/s3transfer/crt.py b/awscli/s3transfer/crt.py index 9105c63baefe..b441d4dbccf5 100644 --- a/awscli/s3transfer/crt.py +++ b/awscli/s3transfer/crt.py @@ -1699,8 +1699,11 @@ def __call__(self, **kwargs): ) except Exception as e: self._osutil.remove_file(self._temp_filename) - # the CRT future has done already at this point - self._coordinator.set_exception(e) + # This runs as an on_done callback, so the transfer is already + # marked complete and the exception has to override that + # result. Otherwise the download reports success having + # written nothing. + self._coordinator.set_exception(e, override=True) class AfterDoneHandler: diff --git a/tests/unit/s3transfer/test_crt.py b/tests/unit/s3transfer/test_crt.py index 428e0bcdde1c..6d1240608b61 100644 --- a/tests/unit/s3transfer/test_crt.py +++ b/tests/unit/s3transfer/test_crt.py @@ -25,7 +25,7 @@ from botocore.session import Session from s3transfer.constants import GB from s3transfer.exceptions import TransferNotDoneError -from s3transfer.utils import CallArgs +from s3transfer.utils import CallArgs, OSUtils from tests import HAS_CRT, FileCreator, mock, requires_crt, unittest @@ -876,3 +876,42 @@ def test_fio_options( mock_s3_crt_client.call_args[1]['fio_options'].direct_io is direct_io ) + + +@requires_crt_pytest +class TestRenameTempFileHandler: + @pytest.fixture + def coordinator(self): + return s3transfer.crt.CRTTransferCoordinator() + + @pytest.fixture + def osutil(self): + return mock.Mock(spec=OSUtils) + + @pytest.fixture + def handler(self, coordinator, osutil): + return s3transfer.crt.RenameTempFileHandler( + coordinator, 'final', 'temp', osutil + ) + + def test_renames_temp_file(self, handler, osutil): + handler(error=None) + osutil.rename_file.assert_called_once_with('temp', 'final') + + def test_removes_temp_file_on_transfer_error(self, handler, osutil): + handler(error=Exception('transfer failed')) + osutil.remove_file.assert_called_once_with('temp') + assert not osutil.rename_file.called + + def test_surfaces_rename_error(self, coordinator, handler, osutil): + osutil.rename_file.side_effect = OSError('Is a directory') + # The handler runs as an on done callback, so the transfer is already + # complete by the time the rename fails. + coordinator.complete() + assert coordinator.done() + + handler(error=None) + + osutil.remove_file.assert_called_once_with('temp') + with pytest.raises(OSError): + coordinator.result()