Handle already-removed pull request labels

algorithms_keeper/api.py

@@ -1,5 +1,8 @@
+import json
 import logging
 import os
+import re
+from http import HTTPStatus
 from typing import Any, Mapping, MutableMapping
 
 from aiohttp import ClientResponse
@@ -80,11 +83,12 @@ async def _request(
         async with self._session.request(
             method, url, headers=headers, data=body
         ) as response:
-            self.log(response, body)
-            return response.status, response.headers, await response.read()
+            response_body = await response.read()
+            self.log(response, body, response_body)
+            return response.status, response.headers, response_body
 
     @staticmethod
-    def log(response: ClientResponse, body: bytes) -> None:  # pragma: no cover
+    def log(response: ClientResponse, body: bytes, response_body: bytes) -> None:
         """Log the request-response cycle for the GitHub API calls made by the bot.
 
         The logger information will be useful to know what actions the bot made.
@@ -102,6 +106,24 @@ def log(response: ClientResponse, body: bytes) -> None:  # pragma: no cover
         else:
             loggerlevel = logger.error
             data = body.decode(UTF_8_CHARSET)
+            # Missing issue labels are expected when webhook handlers overlap.
+            # Keep other 404s and repository-label deletions visible as errors.
+            if (
+                response.status == HTTPStatus.NOT_FOUND
+                and response.method == "DELETE"
+                and re.fullmatch(
+                    r"/repos/[^/]+/[^/]+/issues/\d+/labels/[^/]+", response.url.raw_path
+                )
+            ):
+                try:
+                    response_data = json.loads(response_body)
+                except (json.JSONDecodeError, UnicodeDecodeError):
+                    response_data = None
+                if (
+                    isinstance(response_data, dict)
+                    and response_data.get("message") == "Label does not exist"
+                ):
+                    loggerlevel = logger.info
         version = response.version
         if version is not None:
             version = f"{version.major}.{version.minor}"

algorithms_keeper/utils.py

@@ -15,9 +15,12 @@
 import urllib.parse
 from base64 import b64decode
 from dataclasses import dataclass
+from http import HTTPStatus
 from pathlib import Path
 from typing import Any, Mapping, Optional, Union
 
+from gidgethub import BadRequest
+
 from algorithms_keeper.api import GitHubAPI
 from algorithms_keeper.constants import PR_REVIEW_BODY
 
@@ -121,10 +124,18 @@ async def remove_label_from_pr_or_issue(
     # or issue) at once.
     for label in label_list:
         parse_label = urllib.parse.quote(label)
-        await gh.delete(
-            f"{labels_url}/{parse_label}",
-            oauth_token=await gh.access_token,
-        )
+        try:
+            await gh.delete(
+                f"{labels_url}/{parse_label}",
+                oauth_token=await gh.access_token,
+            )
+        except BadRequest as exc:
+            # Another webhook may have removed the label since this snapshot.
+            if (
+                exc.status_code != HTTPStatus.NOT_FOUND
+                or str(exc) != "Label does not exist"
+            ):
+                raise
 
 
 async def get_user_open_pr_numbers(

tests/test_api.py

@@ -1,3 +1,4 @@
+import logging
 from typing import Any, AsyncGenerator, Awaitable, Callable, Dict
 
 import aiohttp
@@ -81,3 +82,75 @@ async def test_headers_and_log(github_api: GitHubAPI) -> None:
     )
     data, rate_limit, _ = sansio.decipher_response(*resp)
     assert "rate" in data
+
+
+@pytest.mark.asyncio()
+@pytest.mark.parametrize(
+    "request_args, response_args, expected_level",
+    [
+        (
+            ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"),
+            (404, b'{"message": "Label does not exist"}'),
+            logging.INFO,
+        ),
+        (
+            ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"),
+            (404, b'{"message": "Not Found"}'),
+            logging.ERROR,
+        ),
+        (
+            ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"),
+            (403, b'{"message": "Label does not exist"}'),
+            logging.ERROR,
+        ),
+        (
+            ("POST", "/repos/org/repo/issues/1/labels/tests%20are%20failing"),
+            (404, b'{"message": "Label does not exist"}'),
+            logging.ERROR,
+        ),
+        (
+            ("DELETE", "/repos/org/repo/labels/tests%20are%20failing"),
+            (404, b'{"message": "Label does not exist"}'),
+            logging.ERROR,
+        ),
+        (
+            ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"),
+            (404, b"not json"),
+            logging.ERROR,
+        ),
+        (
+            ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"),
+            (404, b"[]"),
+            logging.ERROR,
+        ),
+    ],
+)
+async def test_label_removal_logging(
+    request_args: tuple[str, str],
+    response_args: tuple[int, bytes],
+    expected_level: int,
+    github_api: GitHubAPI,
+    aiohttp_server: Callable[..., Awaitable[TestServer]],
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    response_status, response_body = response_args
+
+    async def handler(request: web.Request) -> web.Response:
+        return web.Response(status=response_status, body=response_body)
+
+    app = web.Application()
+    app.router.add_route("*", "/{path:.*}", handler)
+    server = await aiohttp_server(app)
+    method, path = request_args
+    with caplog.at_level(logging.INFO, logger="algorithms_keeper"):
+        status, _, body = await github_api._request(
+            method, str(server.make_url(path)), {}
+        )
+
+    assert status == response_status
+    assert body == response_body
+    records = [
+        record for record in caplog.records if record.name == "algorithms_keeper"
+    ]
+    assert len(records) == 1
+    assert records[0].levelno == expected_level

tests/test_check_runs.py

@@ -1,6 +1,11 @@
+import asyncio
+from http import HTTPStatus
+from typing import Any
+from unittest.mock import AsyncMock
 from urllib.parse import quote
 
 import pytest
+from gidgethub import BadRequest
 from gidgethub.sansio import Event
 
 from algorithms_keeper.constants import Label
@@ -226,3 +231,54 @@ async def test_check_run(
 ) -> None:
     await check_run_router.dispatch(event, gh)
     assert gh == expected
+
+
+@pytest.mark.asyncio()
+async def test_concurrent_check_runs_remove_same_label(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    gh = MockGitHubAPI(
+        getitem={
+            search_url: {
+                "total_count": 1,
+                "items": [
+                    {"labels": [{"name": Label.FAILED_TEST}], "labels_url": labels_url}
+                ],
+            },
+            check_run_url: {
+                "check_runs": [{"status": "completed", "conclusion": "success"}],
+            },
+        }
+    )
+    deleted = False
+    both_deleting = asyncio.Event()
+
+    async def delete_label(url: str, **kwargs: Any) -> None:
+        nonlocal deleted
+        # Both handlers have read the same label snapshot before either deletes it.
+        if delete.await_count == 2:
+            both_deleting.set()
+        await asyncio.wait_for(both_deleting.wait(), timeout=5)
+        if deleted:
+            raise BadRequest(HTTPStatus.NOT_FOUND, "Label does not exist")
+        deleted = True
+
+    delete = AsyncMock(side_effect=delete_label)
+    monkeypatch.setattr(gh, "delete", delete)
+    events = [
+        Event(
+            data={
+                "action": "completed",
+                "repository": {"full_name": repository},
+                "check_run": {"id": check_id, "head_sha": sha},
+            },
+            event="check_run",
+            delivery_id=f"completed-check-{check_id}",
+        )
+        for check_id in range(2)
+    ]
+
+    await asyncio.gather(*(check_run_router.dispatch(event, gh) for event in events))
+
+    assert deleted
+    assert delete.await_count == 2

tests/test_utils.py

@@ -1,12 +1,16 @@
 import urllib.parse
+from http import HTTPStatus
 from pathlib import Path
 from typing import Dict, cast
+from unittest.mock import AsyncMock
 
 import pytest
+from gidgethub import BadRequest
 
 from algorithms_keeper import utils
 from algorithms_keeper.api import GitHubAPI
 from algorithms_keeper.constants import Label
+from algorithms_keeper.event.pull_request import update_stage_label
 
 from .utils import (
     MockGitHubAPI,
@@ -145,6 +149,70 @@ async def test_remove_multiple_labels() -> None:
     assert f"{labels_url}/{parse_label2}" in gh.delete_url
 
 
+@pytest.mark.asyncio()
+async def test_remove_missing_label_continues(monkeypatch: pytest.MonkeyPatch) -> None:
+    gh = MockGitHubAPI()
+    delete = AsyncMock(
+        side_effect=[BadRequest(HTTPStatus.NOT_FOUND, "Label does not exist"), None]
+    )
+    monkeypatch.setattr(gh, "delete", delete)
+
+    await utils.remove_label_from_pr_or_issue(
+        cast(GitHubAPI, gh),
+        label=[Label.TYPE_HINT, Label.REQUIRE_TEST],
+        pr_or_issue={"issue_url": issue_url},
+    )
+
+    assert [call.args[0] for call in delete.await_args_list] == [
+        f"{labels_url}/{urllib.parse.quote(Label.TYPE_HINT)}",
+        f"{labels_url}/{urllib.parse.quote(Label.REQUIRE_TEST)}",
+    ]
+
+
+@pytest.mark.asyncio()
+async def test_stage_change_with_stale_labels(monkeypatch: pytest.MonkeyPatch) -> None:
+    gh = MockGitHubAPI()
+    monkeypatch.setattr(
+        gh,
+        "delete",
+        AsyncMock(side_effect=BadRequest(HTTPStatus.NOT_FOUND, "Label does not exist")),
+    )
+
+    await update_stage_label(
+        cast(GitHubAPI, gh),
+        pull_request={"issue_url": issue_url, "labels": [{"name": Label.CHANGE}]},
+        next_label=Label.REVIEW,
+    )
+
+    assert gh.post_data == [{"labels": [Label.REVIEW]}]
+
+
+@pytest.mark.asyncio()
+@pytest.mark.parametrize(
+    "status, message",
+    [
+        (HTTPStatus.NOT_FOUND, "Not Found"),
+        (HTTPStatus.FORBIDDEN, "Label does not exist"),
+        (HTTPStatus.FORBIDDEN, "Resource not accessible by integration"),
+    ],
+)
+async def test_remove_label_preserves_other_errors(
+    status: HTTPStatus, message: str, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    gh = MockGitHubAPI()
+    error = BadRequest(status, message)
+    monkeypatch.setattr(gh, "delete", AsyncMock(side_effect=error))
+
+    with pytest.raises(BadRequest) as exc_info:
+        await utils.remove_label_from_pr_or_issue(
+            cast(GitHubAPI, gh),
+            label=Label.FAILED_TEST,
+            pr_or_issue={"issue_url": issue_url},
+        )
+
+    assert exc_info.value is error
+
+
 @pytest.mark.asyncio()
 async def test_get_user_open_pr_numbers() -> None:
     getiter = {