mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-21 02:05:45 +00:00
fix(gateway): correct GitHub auto-retry claim in webhook route docs (#4289)
While investigating a review on PR #4274 (which corrected the same misconception in the channel manager's dedupe-TTL comment), I found a more elaborate version of the same factual error in this route, predating that PR: the docstring claimed "GitHub retries 5xx deliveries with exponential backoff (up to ~5 attempts over ~8 hours)". No such behavior exists. Verified directly against GitHub's documentation: failed deliveries (5xx, timeout, connection error alike) are simply recorded as failed and never automatically redelivered, for both repository and GitHub App webhooks (https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries). Redelivery is always explicit: the "Redeliver" button, the REST API, or an operator's own recovery script (GitHub's own recommended pattern for the latter runs on a 6-hour cron, illustrating this is opt-in operator tooling, not a GitHub-side mechanism). The 503-vs-200 status code choice itself was already correct and is unchanged: 503 keeps a transient failure recorded as failed so it can be recovered via one of the explicit paths above, while 200 correctly marks permanent/non-retryable conditions (disabled channel, unknown event, missing service) as done so no pointless redelivery is invited. Only the rationale text describing *why* was wrong. - github_webhooks.py: reworded the route docstring and four inline comments/log messages that repeated "GitHub retries" / "so GitHub retries" framing, citing the actual documented behavior. - test_github_webhooks.py: renamed test_dispatch_failure_returns_503_so_github_retries to test_dispatch_failure_returns_503_not_200 and reworded its docstring plus two nearby docstrings/comments with the same framing. No assertions changed - same status codes and response bodies checked. - AGENTS.md: corrected the GitHub Webhooks router table row to match. All 44 tests in test_github_webhooks.py pass, plus the full test_github_dispatcher.py / test_github_channel.py / test_github_registry.py / test_channels.py suites (313 total). ruff check and ruff format --check are clean on both touched Python files.
This commit is contained in:
Daoyuan Li
+1
-1
@@ -341,7 +341,7 @@ Localhost persistence deliberately reads the direct request `Host` and ignores `
|
||||
| **Thread Runs** (`/api/threads/{id}/runs`) | `POST /` - create background run; `POST /stream` - create + SSE stream; `POST /wait` - create + block; `POST /regenerate/prepare` - prepare clean input + checkpoint metadata for regenerating the latest assistant answer; `GET /` - list runs; `GET /{rid}` - run details; `POST /{rid}/cancel` - cancel; `GET /{rid}/join` - join SSE; `GET /{rid}/messages` - paginated per-run messages `{data, has_more}`; `GET /{rid}/events` - full event stream; `GET /{rid}/workspace-changes` - workspace/output file change summary and optional diffs; `GET /../messages` - legacy thread message array; `GET /../messages/page` - backward thread-global `seq` history page with middleware/successful-regenerate filtering and page-run-scoped feedback enrichment; `GET /../token-usage` - aggregate tokens |
|
||||
| **Feedback** (`/api/threads/{id}/runs/{rid}/feedback`) | `PUT /` - upsert feedback; `DELETE /` - delete user feedback; `POST /` - create feedback; `GET /` - list feedback; `GET /stats` - aggregate stats; `DELETE /{fid}` - delete specific |
|
||||
| **Runs** (`/api/runs`) | `POST /stream` - stateless run + SSE; `POST /wait` - stateless run + block; `GET /{rid}/messages` - paginated messages by run_id `{data, has_more}` (cursor: `after_seq`/`before_seq`); `GET /{rid}/feedback` - list feedback by run_id |
|
||||
| **GitHub Webhooks** (`/api/webhooks/github`) | `POST /` - receive GitHub App / repo webhook deliveries. Verifies `X-Hub-Signature-256` against `GITHUB_WEBHOOK_SECRET`; exempt from auth + CSRF because authenticity is enforced by HMAC. The route is fail-closed: mounted only when `GITHUB_WEBHOOK_SECRET` is set, or when explicit dev opt-in `DEER_FLOW_ALLOW_UNVERIFIED_GITHUB_WEBHOOKS=1` is set. Recognized events include `ping`, `issues`, `issue_comment`, `pull_request`, `pull_request_review`, and `pull_request_review_comment`; unknown events return 200 with `handled=false`. Fan-out runtime failures return 503 so GitHub retries; permanent/non-retryable conditions such as `channels.github.enabled: false`, unknown events, malformed payloads, or unavailable channel service return 200 with a skipped/handled response. |
|
||||
| **GitHub Webhooks** (`/api/webhooks/github`) | `POST /` - receive GitHub App / repo webhook deliveries. Verifies `X-Hub-Signature-256` against `GITHUB_WEBHOOK_SECRET`; exempt from auth + CSRF because authenticity is enforced by HMAC. The route is fail-closed: mounted only when `GITHUB_WEBHOOK_SECRET` is set, or when explicit dev opt-in `DEER_FLOW_ALLOW_UNVERIFIED_GITHUB_WEBHOOKS=1` is set. Recognized events include `ping`, `issues`, `issue_comment`, `pull_request`, `pull_request_review`, and `pull_request_review_comment`; unknown events return 200 with `handled=false`. Fan-out runtime failures return 503, keeping the delivery recorded as failed for manual/API/scripted redelivery (GitHub does not automatically retry any failed delivery, 5xx included); permanent/non-retryable conditions such as `channels.github.enabled: false`, unknown events, malformed payloads, or unavailable channel service return 200 with a skipped/handled response. |
|
||||
| **GitHub Event-Driven Agents** | Custom agents can declare a `github:` block in their `config.yaml` to bind to repos and event triggers. Webhook fan-out publishes one `InboundMessage` per matching binding to the channel bus; `GitHubChannel` routes those messages through `ChannelManager`. The response `dispatch` summarizes matched/fired/skipped agents. |
|
||||
|
||||
**Workspace change review**: `packages/harness/deerflow/workspace_changes/`
|
||||
|
||||
@@ -183,16 +183,23 @@ async def receive_github_webhook(
|
||||
- Returns ``{"ok": True, ...}`` on successful (or no-op) dispatch so
|
||||
GitHub marks the delivery successful and does not retry.
|
||||
|
||||
**Transient fan-out failures return 503**, not 200. GitHub retries 5xx
|
||||
deliveries with exponential backoff (up to ~5 attempts over ~8 hours)
|
||||
but does *not* retry 200 OK. A transient registry filesystem error or
|
||||
bus publish failure on a 200 path would silently drop a real webhook
|
||||
forever, so we return 503 instead and let GitHub redeliver — by the
|
||||
time the redelivery lands the underlying outage is usually gone. The
|
||||
`is_route_enabled()` startup check still handles *configuration*
|
||||
errors fail-closed (route absent → 404); 503 is reserved for runtime
|
||||
failures GitHub should retry. Permanent / non-retryable conditions
|
||||
(unknown event, missing channel service) keep returning 200.
|
||||
**Transient fan-out failures return 503**, not 200. GitHub does NOT
|
||||
automatically retry a failed delivery of any kind — 5xx, timeout, or
|
||||
connection error are all simply recorded as failed; see
|
||||
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
|
||||
failure into a 200 would silently drop a real webhook forever with no
|
||||
way to recover it. Returning 503 instead keeps the delivery correctly
|
||||
recorded as failed in GitHub's Recent Deliveries / Deliveries API, so
|
||||
an operator's manual "Redeliver" click, a REST API call, or a
|
||||
self-hosted recovery script (the pattern GitHub's own docs recommend)
|
||||
can still recover it — by the time that redelivery lands the
|
||||
underlying outage is usually gone. The `is_route_enabled()` startup
|
||||
check still handles *configuration* errors fail-closed (route absent
|
||||
→ 404); 503 is reserved for runtime failures worth making 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
|
||||
prevented this handler from being mounted when no secret is configured.
|
||||
@@ -266,11 +273,13 @@ async def receive_github_webhook(
|
||||
service = get_channel_service()
|
||||
if service is None:
|
||||
# Permanent state, not a transient failure: ``channels.github``
|
||||
# is not enabled in this deployment. Returning 503 would
|
||||
# condemn GitHub to retry every delivery on backoff for hours
|
||||
# before giving up, all the way to no-op. Stay on 200 and
|
||||
# surface the reason in the response body for operators
|
||||
# checking the redelivery page.
|
||||
# is not enabled in this deployment. Returning 503 would mark
|
||||
# this delivery "failed" and invite a manual "Redeliver" or an
|
||||
# operator's recovery script to retry it — pointlessly, since
|
||||
# every such retry hits this same disabled-channel state until
|
||||
# the config changes. Stay on 200 and surface the reason in
|
||||
# the response body for operators checking the redelivery
|
||||
# page.
|
||||
logger.warning(
|
||||
"github_webhook: channel service not available — no agents fired (delivery=%s event=%s)",
|
||||
x_github_delivery,
|
||||
@@ -290,8 +299,9 @@ async def receive_github_webhook(
|
||||
# back to GitHub via ``gh``, contradicting the documented
|
||||
# off-switch. Bail before ``fanout_event`` so no inbound
|
||||
# ever reaches the bus consumer in ChannelManager. Stay on
|
||||
# 200 (permanent state, not transient) so GitHub doesn't
|
||||
# retry; surface the reason in dispatch_result for the
|
||||
# 200 (permanent state, not transient) rather than mark the
|
||||
# delivery failed and invite a pointless manual/scripted
|
||||
# redelivery; surface the reason in dispatch_result for the
|
||||
# Recent Deliveries panel.
|
||||
logger.info(
|
||||
"github_webhook: channels.github.enabled=false — skipping fan-out (delivery=%s event=%s)",
|
||||
@@ -312,13 +322,18 @@ async def receive_github_webhook(
|
||||
raw_default_mention = github_channel_config.get("default_mention_login")
|
||||
operator_default_mention_login = raw_default_mention.strip() if isinstance(raw_default_mention, str) else None
|
||||
|
||||
# Let fan-out exceptions propagate as 503 so GitHub retries.
|
||||
# Let fan-out exceptions propagate as 503, not 200.
|
||||
# ``fanout_event`` calls the registry (filesystem) and the
|
||||
# message bus; both can fail transiently (disk hiccup, bus
|
||||
# queue full, asyncio cancellation). Swallowing those into a
|
||||
# 200 would permanently drop a real webhook because GitHub
|
||||
# only retries on 5xx. The startup-time ``is_route_enabled``
|
||||
# check still covers fail-closed *configuration* errors.
|
||||
# 200 would permanently drop a real webhook, since GitHub
|
||||
# treats 200 as final success and does not automatically
|
||||
# retry any failure — including 5xx (see
|
||||
# https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries).
|
||||
# 503 keeps the delivery recorded as failed so a manual
|
||||
# "Redeliver", the REST API, or a recovery script can recover
|
||||
# it. The startup-time ``is_route_enabled`` check still
|
||||
# covers fail-closed *configuration* errors.
|
||||
try:
|
||||
dispatch_result = await fanout_event(
|
||||
service.bus,
|
||||
@@ -329,7 +344,7 @@ async def receive_github_webhook(
|
||||
)
|
||||
except Exception as exc: # noqa: BLE001 — re-raised as 503 below
|
||||
logger.exception(
|
||||
"github_webhook: fanout failed (delivery=%s event=%s) — returning 503 so GitHub retries",
|
||||
"github_webhook: fanout failed (delivery=%s event=%s) — returning 503 (recoverable via manual/API redelivery)",
|
||||
x_github_delivery,
|
||||
x_github_event,
|
||||
)
|
||||
|
||||
@@ -542,17 +542,21 @@ def test_dispatch_result_included_in_response(client: TestClient, monkeypatch: p
|
||||
assert fake.await_count == 1
|
||||
|
||||
|
||||
def test_dispatch_failure_returns_503_so_github_retries(client: TestClient, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""A crashing fan-out helper must return 503 so GitHub retries.
|
||||
def test_dispatch_failure_returns_503_not_200(client: TestClient, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""A crashing fan-out helper must return 503, not 200.
|
||||
|
||||
The earlier behaviour swallowed every fan-out exception into a 200 OK
|
||||
response (``dispatch={"error": "fanout failed"}``). GitHub only retries
|
||||
5xx — a 200 ack permanently drops the delivery. The route now lets
|
||||
runtime failures propagate as 503 so a transient registry/bus error
|
||||
triggers GitHub's redelivery path. The startup-time
|
||||
response (``dispatch={"error": "fanout failed"}``). GitHub treats 200
|
||||
as final success and does not automatically retry any failure,
|
||||
including 5xx (see
|
||||
https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries)
|
||||
— so a 200 ack permanently drops the delivery. The route now lets
|
||||
runtime failures propagate as 503 so the delivery is correctly
|
||||
recorded as failed, recoverable via a manual "Redeliver", the REST
|
||||
API, or an operator's own recovery script. The startup-time
|
||||
``is_route_enabled`` check still handles *configuration* failures
|
||||
fail-closed (route absent → 404); 503 is reserved for runtime
|
||||
failures GitHub can retry past.
|
||||
failures worth making recoverable this way.
|
||||
"""
|
||||
|
||||
async def fake_fanout(*args, **kwargs) -> dict:
|
||||
@@ -611,7 +615,8 @@ def test_dispatch_failure_503_lets_github_redeliver_successfully(client: TestCli
|
||||
)
|
||||
assert first.status_code == 503
|
||||
|
||||
# GitHub redelivers — same payload, same signature.
|
||||
# A redelivery — same payload, same signature (e.g. a manual
|
||||
# "Redeliver" click, since GitHub does not resend this automatically).
|
||||
second = client.post(
|
||||
"/api/webhooks/github",
|
||||
content=body,
|
||||
@@ -672,8 +677,9 @@ def test_channel_disabled_skips_fanout(client: TestClient, monkeypatch: pytest.M
|
||||
publishing inbound onto the bus would let the ChannelManager consumer
|
||||
pick it up and run agents that then post back to GitHub via ``gh``,
|
||||
contradicting the documented off-switch. Returns 200 (permanent
|
||||
state, not transient) so GitHub doesn't retry; ``dispatch.skipped``
|
||||
surfaces the reason in the Recent Deliveries panel.
|
||||
state, not transient) rather than mark the delivery failed and invite
|
||||
a pointless redelivery; ``dispatch.skipped`` surfaces the reason in
|
||||
the Recent Deliveries panel.
|
||||
"""
|
||||
bus = MessageBus()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user