From ee2ff7b8fdf14548f16c9950df0c8e6ea70e0aa2 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sat, 19 Sep 2026 14:45:33 +0200 Subject: [PATCH 1/3] Use database deduplication for Bugzilla webhooks --- services/hackbot-api/app/config.py | 5 +--- services/hackbot-api/app/routers/webhooks.py | 22 +++------------- services/hackbot-api/tests/test_webhooks.py | 27 +++++++------------- 3 files changed, 13 insertions(+), 41 deletions(-) diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index bd245620a2..bf244ca99d 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -36,8 +36,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): @@ -80,8 +78,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 4450605353..96846a31ff 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -73,13 +73,6 @@ def get_bugzilla_authorizer(request: Request) -> BugzillaAuthorizer: maxsize=4096, ttl=settings.webhook.dedupe_ttl_seconds ) -# 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", @@ -176,14 +169,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( @@ -200,13 +185,12 @@ 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 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 0ceffa8e84..8bdbf21912 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -532,9 +532,11 @@ class _FakeHackbotClient: def __init__(self): self.calls = [] + self.dedupe_keys = [] - async def trigger_run(self, agent_name, inputs): + async def trigger_run(self, agent_name, inputs, *, dedupe_key=None): self.calls.append((agent_name, inputs)) + self.dedupe_keys.append(dedupe_key) return RunRef( run_id="d3d5f21d-d716-4bb0-a812-8c9ef3e2f1c6", agent=agent_name, @@ -575,7 +577,6 @@ def client(monkeypatch, authorizer, bugzilla_authorizer, phab_client): monkeypatch.setattr(settings.bugzilla_webhook, "bot_login", BUGZILLA_BOT_LOGIN) # Fresh dedupe cache per test. webhooks._seen_transactions.clear() - 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: ( @@ -795,47 +796,37 @@ 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) + _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" 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()) From 3fa0ee2b807ef7769ab76c899ba23a7574b6f297 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Sat, 19 Sep 2026 14:49:52 +0200 Subject: [PATCH 2/3] update the doc --- docs/hackbot/triggers.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/hackbot/triggers.md b/docs/hackbot/triggers.md index d2c06accca..11813cdd6d 100644 --- a/docs/hackbot/triggers.md +++ b/docs/hackbot/triggers.md @@ -131,9 +131,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. From 6ee0e87a4c2126b828ba42bd19735da7877c8670 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 24 Sep 2026 21:30:54 +0200 Subject: [PATCH 3/3] Report duplicate Bugzilla webhook deliveries as ignored --- services/hackbot-api/app/routers/webhooks.py | 12 ++++++++++++ services/hackbot-api/tests/test_webhooks.py | 8 +++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index c062e72415..f704dedaa0 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -197,6 +197,18 @@ async def bugzilla_webhook( }, dedupe_key=f"ni{detected.flag_id}", ) + 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 (flag: %s)", run.run_id, diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index 8c27791145..2f9084695d 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -880,13 +880,19 @@ def test_bugzilla_route_keys_retry_same_but_later_event_differently(client): payload = _bugzilla_payload() first = _post_bugzilla(client, payload) - _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" + # 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 fake_api.dedupe_keys == ["ni2187233", "ni2187233", "ni2187234"]