github/deer-flow
1
0
Fork 0
mirror of https://github.com/bytedance/deer-flow.git synced 2026-08-01 10:56:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(gateway): stop implying 200 webhook deliveries are unrecoverable (#4307)

Browse Source 
PR #4289 corrected the false claim that GitHub auto-retries 5xx webhook
deliveries, but its replacement wording overcorrected: it described a
mistaken 200 response as dropping the webhook "forever with no way to
recover it" / "permanently" - implying manual recovery is impossible,
not just unprompted. fancyboi999 flagged this in a CHANGES_REQUESTED
review on #4289 (submitted 23:21:23Z, referencing
github_webhooks.py:190-197,325-335 and
test_github_webhooks.py:548-559) that went unaddressed before the PR
was approved and merged roughly 40 minutes later.

Verified directly against GitHub's documentation before changing
anything: the manual "Redeliver" button and the REST/App redelivery
endpoints place no failed-status precondition on the delivery id - any
past delivery, success or failure, can be redelivered within GitHub's
~3-day window
(https://docs.github.com/en/webhooks/testing-and-troubleshooting-webhooks/redelivering-webhooks,
https://docs.github.com/en/rest/repos/webhooks#redeliver-a-delivery-for-a-repository-webhook).
The real problem with swallowing a transient failure into 200 is
discoverability, not recoverability: the delivery never shows up as
failed in Recent Deliveries, and GitHub's own recommended
scripted-recovery pattern filters on non-OK status by convention, not
because the platform blocks redelivering a success. A 200'd delivery
can still be redelivered by hand if an operator happens to look - they
just get no signal telling them to, unlike the 503 path, which stays
correctly flagged as failed and so is actually found.

- github_webhooks.py: reworded the route docstring and the inline
  fan-out comment to describe the 200-vs-503 difference as
  discoverability, not raw recoverability, and added the redelivery
  docs link alongside the existing failed-deliveries link.
- test_github_webhooks.py: reworded
  test_dispatch_failure_returns_503_not_200's docstring the same way.
  No assertions changed.

All 44 tests in test_github_webhooks.py pass, plus
test_github_dispatcher.py / test_github_channel.py /
test_github_registry.py / test_channels.py (322 total). ruff check and
ruff format --check are clean on both touched files.
This commit is contained in:
Daoyuan Li 2026-07-22 23:32:49 -07:00 committed by GitHub
parent 70fb91654d
commit 04659cc8dd
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 51 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
backend/app/gateway/routers/github_webhooks.py
View File

@ -187,19 +187,27 @@ async def receive_github_webhook(
automatically retry a failed delivery of any kind — 5xx, timeout, or automatically retry a failed delivery of any kind — 5xx, timeout, or
connection error are all simply recorded as failed; see connection error are all simply recorded as failed; see
https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries. https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries.
A 200 response marks the delivery permanently successful, so Swallowing a transient registry filesystem error or bus publish
swallowing a transient registry filesystem error or bus publish failure into a 200 would mark the delivery successful, so it never
failure into a 200 would silently drop a real webhook forever with no surfaces as failed in GitHub's Recent Deliveries / Deliveries API and
way to recover it. Returning 503 instead keeps the delivery correctly a scheduled recovery script filtering on non-OK status (the pattern
recorded as failed in GitHub's Recent Deliveries / Deliveries API, so GitHub's own docs recommend) will never flag it for redelivery. That
an operator's manual "Redeliver" click, a REST API call, or a is a discoverability problem, not literal unrecoverability — GitHub's
self-hosted recovery script (the pattern GitHub's own docs recommend) manual "Redeliver" button and its REST/App redelivery endpoints place
can still recover it — by the time that redelivery lands the no failed-status precondition on the delivery id (see
underlying outage is usually gone. The `is_route_enabled()` startup https://docs.github.com/en/webhooks/testing-and-troubleshooting-webhooks/redelivering-webhooks),
check still handles *configuration* errors fail-closed (route absent so an operator who independently identifies this exact delivery can
→ 404); 503 is reserved for runtime failures worth making recoverable still redeliver it by hand within GitHub's ~3-day redelivery window —
this way. Permanent / non-retryable conditions (unknown event, missing they just get no automated signal telling them to. Returning 503
channel service) keep returning 200. keeps the delivery correctly recorded as failed instead, so that same
manual click, REST call, or recovery script actually finds it rather
than merely being capable of redelivering it if asked, well inside
the window the underlying outage usually clears in. The
`is_route_enabled()` startup check still handles *configuration*
errors fail-closed (route absent → 404); 503 is reserved for runtime
failures worth making discoverable and recoverable this way.
Permanent / non-retryable conditions (unknown event, missing channel
service) keep returning 200.
The route is fail-closed: :func:`is_route_enabled` should have already The route is fail-closed: :func:`is_route_enabled` should have already
prevented this handler from being mounted when no secret is configured. prevented this handler from being mounted when no secret is configured.
@ -326,14 +334,20 @@ async def receive_github_webhook(
# ``fanout_event`` calls the registry (filesystem) and the # ``fanout_event`` calls the registry (filesystem) and the
# message bus; both can fail transiently (disk hiccup, bus # message bus; both can fail transiently (disk hiccup, bus
# queue full, asyncio cancellation). Swallowing those into a # queue full, asyncio cancellation). Swallowing those into a
# 200 would permanently drop a real webhook, since GitHub # 200 would hide a real failure: GitHub treats 200 as final
# treats 200 as final success and does not automatically # success and never automatically retries any failure,
# retry any failure — including 5xx (see # including 5xx (see
# https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries). # https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries).
# 503 keeps the delivery recorded as failed so a manual # A 200 delivery can still be redelivered by hand — GitHub's
# "Redeliver", the REST API, or a recovery script can recover # manual "Redeliver" button and REST redelivery endpoint have
# it. The startup-time ``is_route_enabled`` check still # no failed-status precondition (see
# covers fail-closed *configuration* errors. # https://docs.github.com/en/webhooks/testing-and-troubleshooting-webhooks/redelivering-webhooks)
# — but nothing flags it for an operator or recovery script
# to do so. 503 keeps the delivery correctly recorded as
# failed so that same manual click, REST call, or recovery
# script actually finds and recovers it. The startup-time
# ``is_route_enabled`` check still covers fail-closed
# *configuration* errors.
try: try:
dispatch_result = await fanout_event( dispatch_result = await fanout_event(
service.bus, service.bus,

25
backend/tests/test_github_webhooks.py
View File

@ -547,16 +547,25 @@ def test_dispatch_failure_returns_503_not_200(client: TestClient, monkeypatch: p
The earlier behaviour swallowed every fan-out exception into a 200 OK The earlier behaviour swallowed every fan-out exception into a 200 OK
response (``dispatch={"error": "fanout failed"}``). GitHub treats 200 response (``dispatch={"error": "fanout failed"}``). GitHub treats 200
as final success and does not automatically retry any failure, as final success and never automatically retries any failure,
including 5xx (see including 5xx (see
https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries) https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries)
— so a 200 ack permanently drops the delivery. The route now lets — so a mistaken 200 ack hides the failure from GitHub's Recent
runtime failures propagate as 503 so the delivery is correctly Deliveries status and from any recovery script filtering on non-OK
recorded as failed, recoverable via a manual "Redeliver", the REST deliveries (the pattern GitHub's own docs recommend). That is a
API, or an operator's own recovery script. The startup-time discoverability gap, not literal unrecoverability: GitHub's manual
``is_route_enabled`` check still handles *configuration* failures "Redeliver" button and its REST redelivery endpoint place no
fail-closed (route absent → 404); 503 is reserved for runtime failed-status precondition on the delivery id (see
failures worth making recoverable this way. https://docs.github.com/en/webhooks/testing-and-troubleshooting-webhooks/redelivering-webhooks),
so a 200'd delivery can still be redelivered by an operator who
independently finds it — they just get no signal to. The route now
lets runtime failures propagate as 503 instead, so the delivery is
correctly recorded as failed and actually surfaces for a manual
"Redeliver" click, the REST API, or an operator's own recovery
script to find and recover. The startup-time ``is_route_enabled``
check still handles *configuration* failures fail-closed (route
absent → 404); 503 is reserved for runtime failures worth making
discoverable and recoverable this way.
""" """
async def fake_fanout(*args, **kwargs) -> dict: async def fake_fanout(*args, **kwargs) -> dict: