From 1da13a054db772e1648cbea36d2f1344b1dc24af Mon Sep 17 00:00:00 2001 From: Gguidini Date: Wed, 4 Oct 2023 15:34:03 -0300 Subject: [PATCH 1/2] chore: handle internal `send_single_upload_put` exceptions Instead of failing (and raising) in the case of a request exception (in `send_single_upload_put`) we will now catch the exception, emit warning, and return safely. This is important because the aggregate of all tasks WILL fail if one of the requests fail. Also notice that `httpx.HTTPError` is the base error class for a number of request-related errors, so this covers many of the failure possibilities here --- .../services/staticanalysis/__init__.py | 45 ++++---- .../test_static_analysis_service.py | 101 +++++++++++++++++- 2 files changed, 126 insertions(+), 20 deletions(-) diff --git a/codecov_cli/services/staticanalysis/__init__.py b/codecov_cli/services/staticanalysis/__init__.py index aa2ab058..95175ae8 100644 --- a/codecov_cli/services/staticanalysis/__init__.py +++ b/codecov_cli/services/staticanalysis/__init__.py @@ -188,35 +188,44 @@ async def process_files( ) -async def send_single_upload_put(client, all_data, el): +async def send_single_upload_put(client, all_data, el) -> typing.Dict: retryable_statuses = (429,) presigned_put = el["raw_upload_location"] number_retries = 5 - for current_retry in range(number_retries): - response = await client.put( - presigned_put, data=json.dumps(all_data[el["filepath"]]) - ) - if response.status_code < 300: - return { - "status_code": response.status_code, - "filepath": el["filepath"], - "succeeded": True, - } - if response.status_code in retryable_statuses: - await asyncio.sleep(2**current_retry) + try: + for current_retry in range(number_retries): + response = await client.put( + presigned_put, data=json.dumps(all_data[el["filepath"]]) + ) + if response.status_code < 300: + return { + "status_code": response.status_code, + "filepath": el["filepath"], + "succeeded": True, + } + if response.status_code in retryable_statuses: + await asyncio.sleep(2**current_retry) + status_code = response.status_code + message_to_warn = response.text + exception = None + except httpx.HTTPError as exp: + status_code = None + exception = type(exp) + message_to_warn = str(exp) logger.warning( - "Unable to send data", + "Unable to send single_upload_put", extra=dict( extra_log_attributes=dict( - response=response.text, + message=message_to_warn, + exception=exception, filepath=el["filepath"], - url=presigned_put, - latest_status_code=response.status_code, + latest_status_code=status_code, ) ), ) return { - "status_code": response.status_code, + "status_code": status_code, + "exception": exception, "filepath": el["filepath"], "succeeded": False, } diff --git a/tests/services/static_analysis/test_static_analysis_service.py b/tests/services/static_analysis/test_static_analysis_service.py index ea443700..fae25226 100644 --- a/tests/services/static_analysis/test_static_analysis_service.py +++ b/tests/services/static_analysis/test_static_analysis_service.py @@ -2,12 +2,17 @@ from unittest.mock import MagicMock import click +import httpx import pytest import requests import responses from responses import matchers -from codecov_cli.services.staticanalysis import process_files, run_analysis_entrypoint +from codecov_cli.services.staticanalysis import ( + process_files, + run_analysis_entrypoint, + send_single_upload_put, +) from codecov_cli.services.staticanalysis.types import ( FileAnalysisRequest, FileAnalysisResult, @@ -67,7 +72,7 @@ def imap_side_effect(mapped_func, files): ) @pytest.mark.asyncio - async def test_static_analysis_service(self, mocker): + async def test_static_analysis_service_success(self, mocker): mock_file_finder = mocker.patch( "codecov_cli.services.staticanalysis.select_file_finder" ) @@ -143,6 +148,98 @@ async def side_effect(*args, **kwargs): "raw_upload_location": "http://storage-url", } + @pytest.mark.asyncio + async def test_send_single_upload_put_success(self, mocker): + mock_client = MagicMock() + + async def side_effect(presigned_put, data): + if presigned_put == "http://storage-url-001": + return httpx.Response(status_code=204) + + mock_client.put.side_effect = side_effect + + all_data = { + "file-001": {"some": "data", "id": "1"}, + "file-002": {"some": "data", "id": "2"}, + "file-003": {"some": "data", "id": "3"}, + } + + success_response = await send_single_upload_put( + mock_client, + all_data=all_data, + el={ + "filepath": "file-001", + "raw_upload_location": "http://storage-url-001", + }, + ) + assert success_response == { + "status_code": 204, + "filepath": "file-001", + "succeeded": True, + } + + @pytest.mark.asyncio + async def test_send_single_upload_put_fail_401(self, mocker): + mock_client = MagicMock() + + async def side_effect(presigned_put, data): + if presigned_put == "http://storage-url-002": + return httpx.Response(status_code=401) + + mock_client.put.side_effect = side_effect + + all_data = { + "file-001": {"some": "data", "id": "1"}, + "file-002": {"some": "data", "id": "2"}, + "file-003": {"some": "data", "id": "3"}, + } + + fail_401_response = await send_single_upload_put( + mock_client, + all_data=all_data, + el={ + "filepath": "file-002", + "raw_upload_location": "http://storage-url-002", + }, + ) + assert fail_401_response == { + "status_code": 401, + "exception": None, + "filepath": "file-002", + "succeeded": False, + } + + @pytest.mark.asyncio + async def test_send_single_upload_put_fail_exception(self, mocker): + mock_client = MagicMock() + + async def side_effect(presigned_put, data): + if presigned_put == "http://storage-url-003": + raise httpx.HTTPError("Some error occured in the request") + + mock_client.put.side_effect = side_effect + + all_data = { + "file-001": {"some": "data", "id": "1"}, + "file-002": {"some": "data", "id": "2"}, + "file-003": {"some": "data", "id": "3"}, + } + + fail_401_response = await send_single_upload_put( + mock_client, + all_data=all_data, + el={ + "filepath": "file-003", + "raw_upload_location": "http://storage-url-003", + }, + ) + assert fail_401_response == { + "status_code": None, + "exception": httpx.HTTPError, + "filepath": "file-003", + "succeeded": False, + } + @pytest.mark.asyncio @pytest.mark.parametrize( "finish_endpoint_response,expected", From 0fed329d434d1e7bb20eddcbbc43a3b4fa218ce7 Mon Sep 17 00:00:00 2001 From: Gguidini Date: Wed, 4 Oct 2023 15:52:32 -0300 Subject: [PATCH 2/2] chore: better handle CancelledError from static analysis We have seen stacktraces of static analysis failing with `asyncio.exceptions.CancelledError: Cancelled by cancel scope 7f7514b8ffa0` I do believe that the initial cause is one of the requests failing, and that cancels every other task, but there's a still possible scenario in which the outer future (`asyncio.gather itself`) gets cancelled. Assuming that it happens and that we do get the same exception bubble up, these changes would let the program exit (more) gracefully. Maybe there are other exceptions there that we should also look for? --- .../services/staticanalysis/__init__.py | 9 ++- .../test_static_analysis_service.py | 71 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/codecov_cli/services/staticanalysis/__init__.py b/codecov_cli/services/staticanalysis/__init__.py index 95175ae8..7a9914a5 100644 --- a/codecov_cli/services/staticanalysis/__init__.py +++ b/codecov_cli/services/staticanalysis/__init__.py @@ -109,7 +109,14 @@ async def run_analysis_entrypoint( for el in files_that_need_upload: all_tasks.append(send_single_upload_put(client, all_data, el)) bar.update(1, all_data[el["filepath"]]) - resps = await asyncio.gather(*all_tasks) + try: + resps = await asyncio.gather(*all_tasks) + except asyncio.CancelledError: + message = ( + "Unknown error cancelled the upload tasks.\n" + + f"Uploaded {len(uploaded_files)}/{len(files_that_need_upload)} files successfully." + ) + raise click.ClickException(message) for resp in resps: if resp["succeeded"]: uploaded_files.append(resp["filepath"]) diff --git a/tests/services/static_analysis/test_static_analysis_service.py b/tests/services/static_analysis/test_static_analysis_service.py index fae25226..08876634 100644 --- a/tests/services/static_analysis/test_static_analysis_service.py +++ b/tests/services/static_analysis/test_static_analysis_service.py @@ -1,3 +1,4 @@ +from asyncio import CancelledError from pathlib import Path from unittest.mock import MagicMock @@ -148,6 +149,76 @@ async def side_effect(*args, **kwargs): "raw_upload_location": "http://storage-url", } + @pytest.mark.asyncio + async def test_static_analysis_service_CancelledError(self, mocker): + mock_file_finder = mocker.patch( + "codecov_cli.services.staticanalysis.select_file_finder" + ) + mock_send_upload_put = mocker.patch( + "codecov_cli.services.staticanalysis.send_single_upload_put" + ) + + async def side_effect(client, all_data, el): + if el["filepath"] == "samples/inputs/sample_001.py": + return { + "status_code": 204, + "filepath": el["filepath"], + "succeeded": True, + } + raise CancelledError("Pretending something cancelled this task") + + mock_send_upload_put.side_effect = side_effect + + files_found = map( + lambda filename: FileAnalysisRequest(str(filename), Path(filename)), + [ + "samples/inputs/sample_001.py", + "samples/inputs/sample_002.py", + ], + ) + mock_file_finder.return_value.find_files = MagicMock(return_value=files_found) + with responses.RequestsMock() as rsps: + rsps.add( + responses.POST, + "https://api.codecov.io/staticanalysis/analyses", + json={ + "external_id": "externalid", + "filepaths": [ + { + "state": "created", + "filepath": "samples/inputs/sample_001.py", + "raw_upload_location": "http://storage-url-001", + }, + { + "state": "created", + "filepath": "samples/inputs/sample_002.py", + "raw_upload_location": "http://storage-url-002", + }, + ], + }, + status=200, + match=[ + matchers.header_matcher({"Authorization": "Repotoken STATIC_TOKEN"}) + ], + ) + + with pytest.raises(click.ClickException) as exp: + await run_analysis_entrypoint( + config={}, + folder=".", + numberprocesses=1, + pattern="*.py", + token="STATIC_TOKEN", + commit="COMMIT", + should_force=False, + folders_to_exclude=[], + enterprise_url=None, + ) + assert "Unknown error cancelled the upload tasks." in str(exp.value) + mock_file_finder.assert_called_with({}) + mock_file_finder.return_value.find_files.assert_called() + assert mock_send_upload_put.call_count == 2 + @pytest.mark.asyncio async def test_send_single_upload_put_success(self, mocker): mock_client = MagicMock()