diff --git a/docs/hackbot/triggers.md b/docs/hackbot/triggers.md index 689f4495f3..0278c56b0f 100644 --- a/docs/hackbot/triggers.md +++ b/docs/hackbot/triggers.md @@ -128,9 +128,9 @@ Guards, each closing a specific failure mode: - **Private bugs are never processed** — the check is `is_private is not False`, so a missing or non-boolean value fails closed. - **Dedupe** — retried deliveries are deduped by the **needinfo flag id**, which is globally - unique, so a later needinfo on the same bug gets a new id and still triggers. As on the - Phabricator side, the key is claimed **only after a successful trigger**, keeping a - transient failure retryable by BMO. + unique, so a later needinfo on the same bug gets a new id and still triggers. The flag id + is used as the run's database dedupe key, so retries reuse the existing run even after a + service restart. - **Latest flag wins** — BMO orders flags by id, so the last matching one is the newly requested one. diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index 78f884b65d..7c72630bf5 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -34,8 +34,6 @@ class BugzillaWebhookSettings(BaseModel): secret: str # The Bugzilla account to which the needinfo request must be directed. bot_login: str = "hackbot@mozilla.tld" - # Best-effort in-memory dedupe of retried bug-modification deliveries. - dedupe_ttl_seconds: int = 6 * 60 * 60 class SlackSettings(BaseModel): @@ -79,8 +77,7 @@ class Settings(BaseSettings): webhook: WebhookSettings # Bugzilla uses a separate shared-secret header and bot identity. These map - # from BUGZILLA_WEBHOOK_SECRET, BUGZILLA_WEBHOOK_BOT_LOGIN, and - # BUGZILLA_WEBHOOK_DEDUPE_TTL_SECONDS. + # from BUGZILLA_WEBHOOK_SECRET and BUGZILLA_WEBHOOK_BOT_LOGIN. bugzilla_webhook: BugzillaWebhookSettings bugzilla_api_url: str = "https://bugzilla.mozilla.org/rest" diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index 2c006c9533..f704dedaa0 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -2,7 +2,6 @@ import logging -from cachetools import TTLCache from fastapi import APIRouter, Depends, Request, Response, status from hackbot_client import HackbotClient from phabricator_client import PhabricatorClient @@ -89,14 +88,6 @@ def get_bugzilla_authorizer(request: Request) -> BugzillaAuthorizer: return authorizer -# Best-effort dedupe of retried BMO deliveries, keyed by the globally unique -# needinfo flag ID. A later needinfo on the same bug receives a new flag ID. -# TODO: Replace with DB-level deduplication (#6716). -_seen_bugzilla_events: TTLCache = TTLCache( - maxsize=4096, ttl=settings.bugzilla_webhook.dedupe_ttl_seconds -) - - @router.post( "/phabricator", status_code=status.HTTP_202_ACCEPTED, @@ -188,14 +179,6 @@ async def bugzilla_webhook( ) if detected is None: return {"status": "ignored", "reason": "no actionable Hackbot needinfo"} - dedupe_key = f"ni{detected.flag_id}" - if dedupe_key in _seen_bugzilla_events: - log.info( - "Ignored duplicate Bugzilla needinfo webhook for bug %s (flag: %s)", - detected.bug_id, - detected.flag_id, - ) - return {"status": "ignored", "reason": "duplicate delivery"} if not await authorizer.is_authorized(detected.user_login): log.info( @@ -212,13 +195,24 @@ async def bugzilla_webhook( "bugzilla_needinfo_flag_id": detected.flag_id, "comment": detected.comment, }, + dedupe_key=f"ni{detected.flag_id}", ) - # Do not consume an event until run creation succeeds; a transient failure - # must remain retryable by Bugzilla. - _seen_bugzilla_events[dedupe_key] = True + if not run.is_new: + log.info( + "Duplicate Bugzilla delivery for bug %s (flag: %s) resolved to run %s", + detected.bug_id, + detected.flag_id, + run.run_id, + ) + return { + "status": "ignored", + "reason": "duplicate delivery", + "run_id": run.run_id, + } log.info( - "Triggered bug-fix run %s for Bugzilla bug %s from needinfo request", + "Triggered bug-fix run %s for Bugzilla bug %s from needinfo request (flag: %s)", run.run_id, detected.bug_id, + detected.flag_id, ) return {"status": "triggered", "run_id": run.run_id} diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index bed235dc64..2f9084695d 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -614,8 +614,6 @@ def client(monkeypatch, authorizer, bugzilla_authorizer, phab_client): monkeypatch.setattr(settings.webhook, "secret", SECRET) monkeypatch.setattr(settings.bugzilla_webhook, "secret", BUGZILLA_SECRET) monkeypatch.setattr(settings.bugzilla_webhook, "bot_login", BUGZILLA_BOT_LOGIN) - # Fresh dedupe cache per test. - webhooks._seen_bugzilla_events.clear() app.dependency_overrides[webhooks.get_phabricator_client] = lambda: phab_client app.dependency_overrides[webhooks.get_phabricator_authorizer] = lambda: authorizer app.dependency_overrides[webhooks.get_bugzilla_authorizer] = lambda: ( @@ -868,47 +866,43 @@ def test_bugzilla_route_ignores_unauthorized_actor(client, bugzilla_authorizer): fake_api = _FakeHackbotClient() app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api payload = _bugzilla_payload() - detected = detect_needinfo_request(payload, bot_login=BUGZILLA_BOT_LOGIN) response = _post_bugzilla(client, payload) assert response.status_code == 202 assert response.json() == {"status": "ignored", "reason": "unauthorized user"} assert fake_api.calls == [] - # The event stays unconsumed: the same flag can still trigger a run once - # the actor is authorized. - assert f"ni{detected.flag_id}" not in webhooks._seen_bugzilla_events -def test_bugzilla_route_dedupes_retry_but_not_later_event(client): +def test_bugzilla_route_keys_retry_same_but_later_event_differently(client): fake_api = _FakeHackbotClient() app.dependency_overrides[webhooks.get_hackbot_client] = lambda: fake_api payload = _bugzilla_payload() first = _post_bugzilla(client, payload) - duplicate = _post_bugzilla(client, payload) + retry = _post_bugzilla(client, payload) later = _post_bugzilla( client, _bugzilla_payload(flag_id=2187234, event_time="2026-08-07T19:00:05"), ) assert first.json()["status"] == "triggered" - assert duplicate.json()["reason"] == "duplicate delivery" + # The retry is answered with the existing run rather than claimed as new. + assert retry.json() == { + "status": "ignored", + "reason": "duplicate delivery", + "run_id": "d3d5f21d-d716-4bb0-a812-8c9ef3e2f1c6", + } assert later.json()["status"] == "triggered" - assert len(fake_api.calls) == 2 + assert fake_api.dedupe_keys == ["ni2187233", "ni2187233", "ni2187234"] -def test_bugzilla_route_does_not_dedupe_failed_dispatch(client): +def test_bugzilla_route_surfaces_failed_dispatch(client): class _FailingClient: - async def trigger_run(self, agent_name, inputs): + async def trigger_run(self, agent_name, inputs, *, dedupe_key=None): raise RuntimeError("run creation failed") - payload = _bugzilla_payload() - detected = detect_needinfo_request(payload, bot_login=BUGZILLA_BOT_LOGIN) - assert detected is not None app.dependency_overrides[webhooks.get_hackbot_client] = lambda: _FailingClient() with pytest.raises(RuntimeError, match="run creation failed"): - _post_bugzilla(client, payload) - - assert f"ni{detected.flag_id}" not in webhooks._seen_bugzilla_events + _post_bugzilla(client, _bugzilla_payload())