chore: better handle CancelledError from static analysis (#275)

* 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

* 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?

Author: giovanni-guidini

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"])
@@ -188,35 +195,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,
     }

tests/services/static_analysis/test_static_analysis_service.py

@@ -1,13 +1,19 @@
+from asyncio import CancelledError
 from pathlib import Path
 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 +73,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 +149,168 @@ 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()
+
+        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",