From eac36a5f53c5d30aac3b04ed5f596d952a0abb30 Mon Sep 17 00:00:00 2001 From: quadseven <59060157+quadseven@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:19:12 -0400 Subject: [PATCH] feat: a dead LLM backend pages an operator instead of failing a review "Why are we using OpenRouter" turned out to have an uncomfortable answer: we are, and it has not worked for a week. Measured 2026-07-27..08-03 from grug's own logs: llm_backend_http_failed openrouter http_402 x4 (out of credits) llm_backend_http_failed poolside http_404 x3 (dead endpoint) OpenRouter and Poolside are Elder's overload valve - the deliberate 2026-07-14 call so a Cave outage does not leave a review `all_failed`. BOTH halves were down at once. So the valve was not degraded, it was absent: any Cave blip went straight to "Grug could not review this", and the only person told was the PR author, who can neither pay an invoice nor rotate a key. Nothing alerted because the only signal was `llm_backend_http_failed`, which also fires for ordinary 429/5xx overload. Alerting on that token would page for the system working as designed, so it never was. So: split the classes. - `is_terminal_backend_failure` - 401/403 (key wrong or revoked), 402 (unpaid), 404 (endpoint or model gone). None of these are fixed by retrying or by falling back. - Terminal failures log `llm_backend_unusable` at ERROR with `failure_class:config_or_billing`. 429/5xx keep the existing token and stay a WARNING. - A digest-tier monitor keys on the new token only. Digest, not page: the Cave still reviews, so this is not a user-facing outage - it is the safety net going missing, which you want to see before the next outage rather than during it. - Runbook section, including that the two backends are NOT interchangeable (different models), and that judge/teller/ask use them as their PRIMARY backend, so a dead one degrades those directly rather than only removing a fallback. This is the first slice of #818. It does not yet stop an author ever seeing "skipped" - it makes the most common CAUSE of it visible to someone who can fix it. Suites: 1886 webhook, 288 api, 25 infra. Refs #818 --- docs/RUNBOOK.md | 49 +++++++++++++++++ infra/pulumi/components/dd_monitors.py | 50 +++++++++++++++++ infra/pulumi/tests/test_dd_monitors.py | 26 +++++++++ services/_shared/llm_client.py | 61 ++++++++++++++++++--- services/webhook/tests/test_llm_client.py | 65 +++++++++++++++++++++++ 5 files changed, 245 insertions(+), 6 deletions(-) diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md index 0ae2465e..c168015a 100644 --- a/docs/RUNBOOK.md +++ b/docs/RUNBOOK.md @@ -670,3 +670,52 @@ the two ways a reporting repo could go silent, so this should now only happen when a repo leaves the estate entirely - deleted, or its installation removed. That case is inherent and bounded: it needs a repo to disappear while red, and it clears itself within 24h. + +## LLM backend unusable + +The `[grug] LLM backend unusable` monitor fires when a review backend answers +401, 402, 403 or 404. That is not overload - it is broken. + +| Status | Meaning | Fix | +|---|---|---| +| 401 / 403 | API key wrong or revoked | Rotate the key in SSM, redeploy | +| 402 | Unpaid / out of credits | Top up the account | +| 404 | Endpoint or model name gone | Check the vendor changed neither | + +### Why it matters even though reviews still work + +The Cave (self-hosted Spark models) is Elder's PRIMARY path. OpenRouter and +Poolside are a bounded, single-shot **overload valve** behind it - the +deliberate 2026-07-14 call to stop a Cave outage leaving a review +`all_failed`. + +So a dead SaaS backend does not fail reviews on its own. It removes the +safety net, silently. The next Cave blip then goes straight to "Grug could +not review this", and the only person told is the PR author, who cannot pay +an invoice or rotate a key. + +That is exactly what happened between 2026-07-27 and 08-03: OpenRouter +returned `http_402` four times and Poolside `http_404` three times. Both +halves of the valve were down at once and nothing said so, because the only +log token was `llm_backend_http_failed`, which also fires for ordinary 429s +and is therefore not alertable. + +### Triage + +```bash +pup logs search --query "llm_backend_unusable" \ + --from "$(date -u -v-24H +%Y-%m-%dT%H:%M:%SZ)" --to "$(date -u +%Y-%m-%dT%H:%M:%SZ)" +``` + +The log carries `backend`, `status` and `failure_class:config_or_billing`. + +Note the two backends are **not** interchangeable: they run different models +(OpenRouter serves Claude, Poolside serves Laguna), so losing one halves the +fallback rather than degrading it evenly. + +### Check the other personas too + +`judge_findings`, `summarize_pr` and `answer_pr_question` use Poolside and +OpenRouter as their **primary** backend via `select_backend`'s per-install +round robin - not as a fallback. A dead SaaS backend degrades those directly, +not just Elder's safety net. diff --git a/infra/pulumi/components/dd_monitors.py b/infra/pulumi/components/dd_monitors.py index b0db07fc..23c4a5f3 100644 --- a/infra/pulumi/components/dd_monitors.py +++ b/infra/pulumi/components/dd_monitors.py @@ -73,6 +73,7 @@ class _MonitorBundle: persona_dispatch_unhandled: datadog.Monitor elder_llm_degraded: datadog.Monitor enforcement_gap: datadog.Monitor + backend_unusable: datadog.Monitor cf_secret_mismatch: datadog.Monitor uptime: datadog.SyntheticsTest credential_acquisition_fail: datadog.Monitor @@ -156,6 +157,29 @@ def poller_cronjob_unhealthy_query() -> str: ) +def backend_unusable_query(env: str) -> str: + """An LLM backend that is not busy but BROKEN - unpaid, revoked, gone. + + Keys on `llm_backend_unusable` (401/402/403/404), never on + `llm_backend_http_failed`, which also carries ordinary 429/5xx overload - + the exact condition the SaaS fallback exists to absorb. Alerting on that + would page for the system working as designed, so it would be muted, so + the real signal would be lost with it. + + Why this monitor exists (#818): between 2026-07-27 and 08-03 OpenRouter + answered `http_402` four times (out of credits) and Poolside `http_404` + three times (dead endpoint). BOTH halves of Elder's overload valve were + down simultaneously. The valve's whole job is to stop a Cave outage + failing a review, so with it dead a Cave blip went straight to "Grug + could not review this" - and the only person told was the PR author, who + can do nothing about an unpaid invoice. + """ + return ( + f'logs("service:grug-* env:{env} llm_backend_unusable")' + '.index("*").rollup("count").last("15m") > 0' + ) + + def enforcement_gap_query(env: str) -> str: """Any repo grug cannot prove is gated by the DoR check. @@ -759,6 +783,31 @@ def create_all( # the DoR check, for >1h. DogStatsD metric emitted by enforcement.py. # Thresholds on the value, never on the enforcement_type tag: see # enforcement_gap_query for why the tag filter latched this (#716). + # An LLM backend that is BROKEN rather than busy. Digest tier on purpose: + # it is not a user-facing outage - Elder still reviews from the Cave - but + # it silently removes the fallback, so the next Cave blip becomes a failed + # review with no safety net. Something to see, not something to wake for. + backend_unusable = datadog.Monitor( + "grug-backend-unusable", + type="log alert", + name="[grug] LLM backend unusable (unpaid / revoked / gone)", + message=( + f"{_DIGEST}\n" + "A review backend returned 401/402/403/404 - it is not overloaded, " + "it is unusable. 402 = unpaid, 401/403 = key wrong or revoked, " + "404 = endpoint or model name gone.\n" + "This does NOT fail reviews on its own: the Cave is the primary " + "path. It removes the FALLBACK, so the next Cave outage has " + "nothing behind it and authors get told Grug could not review.\n" + "Runbook: docs/RUNBOOK.md#llm-backend-unusable" + ), + query=backend_unusable_query(env), + tags=_common_tags(env, "grug-consumer") + ["llm:backend"], + notify_no_data=False, + priority=3, + opts=opts, + ) + enforcement_gap = datadog.Monitor( "grug-enforcement-gap", type="metric alert", @@ -861,6 +910,7 @@ def create_all( persona_dispatch_unhandled=persona_dispatch_unhandled, elder_llm_degraded=elder_llm_degraded, enforcement_gap=enforcement_gap, + backend_unusable=backend_unusable, cf_secret_mismatch=cf_secret_mismatch, uptime=uptime, credential_acquisition_fail=credential_acquisition_fail, diff --git a/infra/pulumi/tests/test_dd_monitors.py b/infra/pulumi/tests/test_dd_monitors.py index 151fb0b0..50737435 100644 --- a/infra/pulumi/tests/test_dd_monitors.py +++ b/infra/pulumi/tests/test_dd_monitors.py @@ -506,3 +506,29 @@ def test_enforcement_gap_query_is_env_scoped() -> None: """Each stack alerts only on its own repos.""" assert "env:dev" in enforcement_gap_query("dev") assert "env:prod" not in enforcement_gap_query("dev") + + +# --- #818: a dead LLM backend must reach an operator ------------------------ + + +def test_backend_unusable_query_targets_the_terminal_token_only() -> None: + """A 402 removed Elder's whole fallback and nobody was told. + + Measured 2026-07-27..08-03: OpenRouter `http_402` x4 (unpaid) and + Poolside `http_404` x3 (dead endpoint) - BOTH halves of the SaaS + overload valve down at once, so a Cave blip went straight to "Grug + could not review this" and the only person informed was the PR author, + who cannot pay a bill. + + Must key on `llm_backend_unusable`, never `llm_backend_http_failed`: + the latter also fires for ordinary 429/5xx overload, which is the + system working as designed. + """ + from components.dd_monitors import backend_unusable_query + + q = backend_unusable_query("prod") + assert "llm_backend_unusable" in q + assert "llm_backend_http_failed" not in q, ( + "alerting on the overload token would be pure noise" + ) + assert "env:prod" in q diff --git a/services/_shared/llm_client.py b/services/_shared/llm_client.py index b840c701..bdef3eb4 100644 --- a/services/_shared/llm_client.py +++ b/services/_shared/llm_client.py @@ -770,6 +770,57 @@ def _saas_overload_fallback_config(backend: Backend) -> BackendConfig: ) +# A backend that answers with one of these is not busy - it is UNUSABLE, and +# no amount of retrying or falling back changes that. 401/403 = the key is +# wrong or revoked. 402 = the bill is unpaid. 404 = the endpoint or model name +# is gone. All four are operator problems with an operator fix. +_TERMINAL_BACKEND_STATUSES = frozenset({401, 402, 403, 404}) + + +def is_terminal_backend_failure(status: int) -> bool: + """Is this a config/billing failure rather than overload? + + The distinction is load-bearing for #818. Measured 2026-07-27..08-03: + OpenRouter returned `http_402` (out of credits) four times and Poolside + `http_404` three times - BOTH halves of the SaaS overload valve were dead + at once. The valve exists precisely so a Cave outage does not fail a + review, so with both dead a Cave blip went straight to "Grug could not + review this", and the only person told was the PR author, who can do + nothing about an unpaid bill. + + 429 and 5xx are deliberately NOT terminal: those are the overload this + fallback was built for, and paging on them would be paging on the system + working as designed. + """ + return status in _TERMINAL_BACKEND_STATUSES + + +def _log_backend_failure(*, backend_value: str, status: int, error: str) -> None: + """Log a backend HTTP failure under a token that matches its KIND. + + Terminal failures get their own token so a monitor can alert on them. + Alerting on `llm_backend_http_failed` is not an option - it fires for + ordinary overload too, so it would be pure noise, which is why a dead + OpenRouter key sat unnoticed while it silently removed Elder's fallback. + """ + if is_terminal_backend_failure(status): + log.error( + "llm_backend_unusable", + extra={ + "backend": backend_value, + "status": status, + "error": error, + # Named so the runbook and the monitor agree on vocabulary. + "failure_class": "config_or_billing", + }, + ) + return + log.warning( + "llm_backend_http_failed", + extra={"backend": backend_value, "status": status, "error": error}, + ) + + def select_backend(installation_id: int) -> Backend: """Stable per-install backend pick via `installation_id % 2`. @@ -2716,9 +2767,8 @@ def _run_review_arm( backend=backend, kind="parse_failed", model=model, error_text=f"{backend.value}: parse_failed: {err}", parse_err=err, ) - log.warning( - "llm_backend_http_failed", - extra={"backend": backend.value, "status": resp.status_code, "error": err}, + _log_backend_failure( + backend_value=backend.value, status=resp.status_code, error=err, ) return _ArmOutcome( backend=backend, kind="http_failed", @@ -3172,9 +3222,8 @@ def _review_diff_dispatch( first_parse_fail = (backend, model, err) last_error = f"{backend.value}: parse_failed: {err}" continue - log.warning( - "llm_backend_http_failed", - extra={"backend": backend.value, "status": resp.status_code, "error": err}, + _log_backend_failure( + backend_value=backend.value, status=resp.status_code, error=err, ) last_error = f"{backend.value}: {err}" diff --git a/services/webhook/tests/test_llm_client.py b/services/webhook/tests/test_llm_client.py index 5dd478d9..18fc82e6 100644 --- a/services/webhook/tests/test_llm_client.py +++ b/services/webhook/tests/test_llm_client.py @@ -2835,3 +2835,68 @@ def test_render_learnings_block_redacts_secret_before_truncation() -> None: rows = [{"text": f"allow key {fake} in fixtures", "scope_path": ""}] block = lc._render_learnings_block(rows) assert fake not in block # redacted before it reached the block + + +# --- #818: a dead backend must page an operator, not degrade a review ------- + + +def test_billing_and_auth_failures_are_classified_terminal(): + """A 402 is not overload - it is an unpaid bill, and no retry fixes it. + + Measured 2026-07-27..2026-08-03: OpenRouter returned `http_402` four + times and Poolside `http_404` three times. Both halves of the SaaS + overload valve were dead simultaneously, so a Cave failure had NO + fallback - and the only visible consequence was an author being told + Grug could not review their PR. + """ + from llm_client import is_terminal_backend_failure + + for status in (401, 402, 403, 404): + assert is_terminal_backend_failure(status) is True, status + + +def test_overload_and_transport_failures_are_not_terminal(): + """429/5xx are exactly what the fallback exists for. Treating them as + config failures would page an operator for normal load.""" + from llm_client import is_terminal_backend_failure + + for status in (408, 429, 500, 502, 503, 504): + assert is_terminal_backend_failure(status) is False, status + + +def test_terminal_backend_failure_emits_its_own_log_token(): + """It needs a DISTINCT token so a monitor can alert on it. + + `llm_backend_http_failed` fires for ordinary overload too, so alerting + on it would be pure noise. A dead key or a dead endpoint is an operator + problem and must be separable. + """ + import logging + + import llm_client + + records = [] + + class _Cap(logging.Handler): + def emit(self, record): + records.append(record.getMessage()) + + lg = logging.getLogger("grug.llm_client") + h = _Cap() + lg.addHandler(h) + try: + llm_client._log_backend_failure( + backend_value="openrouter", status=402, error="insufficient credits", + ) + llm_client._log_backend_failure( + backend_value="openrouter", status=503, error="overloaded", + ) + finally: + lg.removeHandler(h) + + assert "llm_backend_unusable" in records, ( + "a billing/auth/config failure needs its own alertable token" + ) + assert "llm_backend_http_failed" in records, ( + "ordinary overload keeps the existing token" + )