From c3d6bc852620fa06a922220a1f154bfa3dff0fbc Mon Sep 17 00:00:00 2001 From: engineer Date: Fri, 14 Aug 2026 06:45:10 -0700 Subject: [PATCH] fix(diagnostics): classify 401/403 as auth-failed so a wrong password stops the retry loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGE-107. The 498 `API Error: 401` events from one device were not a client token-refresh loop. Sentry breadcrumbs on the surviving events show a `touch` event immediately before every capture, at irregular human-paced intervals (87s, 199s, 69s, 5s, 61s, 66s) — a person re-tapping Connect, not a backoff timer. The app's automated loops were already correct: events.ts terminates the SSE reconnect loop on ApiAuthError (issue #76). What actually drove it: in v0.4.4 the connection probe counted any HTTP response as a successful health check, so a 401 was classified `ok` and shown to the user as "Health endpoint responded — connection actually works now" while their password was wrong. The user retried for two months. `requireOk` (#114, v0.4.8) stopped the false success, but 401 then fell into the generic `health-failed` bucket — "Likely wrong path, auth, or an old server version" — which still doesn't tell anyone to fix their password. - New `auth-failed` classification: a 401/403 from /global/health means the server is up and reachable and rejected the credentials. Its summary names the status, points at the password and OPENCODE_SERVER_USERNAME, and says the server is fine. It flows straight into the existing failure Alert on both the add and edit connection screens — which is where the password field is, i.e. the re-auth prompt. - It short-circuits before the root/internet probes can downgrade it: a 401 already proves the server answered. - `health-failed` copy no longer blames auth. - `connect auth-failed` joins the noise-gate drop-list. A wrong password is user config, unactionable server-side, already visible in the UI and already trended in PostHog as connection_failed{error_class:"unauthorized"}. `health-failed` and `tls-error` still report. Tests: 6 new (401/403 -> auth-failed, message content, root-unreachable does not override, 404/500/502 stay health-failed, health-failed copy drops "auth", noise gate drops `connect auth-failed` but not a raw `API Error: 401`). 263 pass, tsc --noEmit clean. Co-Authored-By: Paperclip --- docs/analytics.md | 20 ++++++++--- src/lib/diagnostics-classify.test.ts | 50 +++++++++++++++++++++++++++- src/lib/diagnostics-classify.ts | 24 ++++++++++++- src/lib/sentry-noise.test.ts | 10 ++++++ src/lib/sentry-noise.ts | 22 ++++++++---- src/lib/sentry.ts | 11 +++--- 6 files changed, 120 insertions(+), 17 deletions(-) diff --git a/docs/analytics.md b/docs/analytics.md index 0dbc86d3..d07ff6e4 100644 --- a/docs/analytics.md +++ b/docs/analytics.md @@ -81,20 +81,32 @@ Consent decides *whether* we report; the noise gate in `src/lib/sentry-noise.ts` often*. It exists because this app became the org's #1 Sentry volume source (~4,500 events/month against a 3,500/month org quota) while ~1,100 of those events were three non-defects: `connect timeout`, `connect server-unreachable`, and one device's -`API Error: 401` token-refresh loop firing 498 times. +`API Error: 401` firing 498 times. + +> **AGE-107 postscript.** That 401 storm was traced to a *human* retry loop, not a client +> token-refresh loop. In v0.4.4 the connection probe scored **any** HTTP response as a +> success, so a 401 was reported to the user as "Health endpoint responded — connection +> actually works now" while their password was wrong. They re-tapped Connect for two months +> (Sentry breadcrumbs show a `touch` event before every single capture, at irregular +> human-paced intervals). `requireOk` in `diagnostics.ts` (v0.4.8) stopped the false +> success; `auth-failed` now gives it its own actionable message and drop-list entry. +> The client's automated loops were never at fault — `events.ts` already terminates the SSE +> reconnect loop on `ApiAuthError` (issue #76). `beforeSend` applies three layers, cheapest first: | Layer | Rule | Effect | |---|---|---| | Always-send allowlist | OOM / ANR / native / `IllegalStateException` / `NullPointerException` / fatal level / unhandled mechanism | Bypasses every limit below — quota is worthless if it silences real crashes | -| Transport drop-list | `connect timeout\|server-unreachable\|no-internet\|malformed-url`, `Network request failed`, `Request timed out after`, `ECONN*`/`ETIMEDOUT`… | Hard drop. Not sampled: the gate is per-install, so even 1/device/day multiplies by the install base back into thousands/month | +| Transport drop-list | `connect timeout\|server-unreachable\|no-internet\|malformed-url\|auth-failed`, `Network request failed`, `Request timed out after`, `ECONN*`/`ETIMEDOUT`… | Hard drop. Not sampled: the gate is per-install, so even 1/device/day multiplies by the install base back into thousands/month | | Dedup + rate cap | per-fingerprint cooldown 6h, ≤6 new fingerprints/h, ≤10 events/h (mirrors the `openclaw-box-bot` shim, AGE-55) | Turns a retry loop into one report and caps any future regression | Nothing is lost by the transport drop: those failures are already shown to the user as connection UI **and** already trended, PII-free, as the PostHog `connection_failed` event with -an `error_class` property (`src/lib/analytics-classify.ts`). Sentry was paying per event for a -graph we already have. +an `error_class` property (`src/lib/analytics-classify.ts` — a 401 lands in `unauthorized`). +Sentry was paying per event for a graph we already have. `connect health-failed` and +`connect tls-error` are deliberately **not** dropped: a box that answers but is unhealthy, or +a broken certificate, is actionable. Dropped-event counts are not silent — the number dropped since the last delivered event rides along as a `noise.dropped_since_last` tag, so the saving is auditable from Sentry itself. diff --git a/src/lib/diagnostics-classify.test.ts b/src/lib/diagnostics-classify.test.ts index 7f60fc38..16dee493 100644 --- a/src/lib/diagnostics-classify.test.ts +++ b/src/lib/diagnostics-classify.test.ts @@ -116,13 +116,61 @@ test("root reachable but internet probe down still classifies as health-failed, // this must not be misreported as "no internet". const r = classify( okUrl, - probe({ ok: false, error: "HTTP 401", status: 401 }), // health + probe({ ok: false, error: "HTTP 404", status: 404 }), // health probe({ ok: false }), // internet (down) probe({ ok: true }), // root (reachable) ) assert.equal(r.classification, "health-failed") }) +// --- AGE-107: a rejected credential must not read as anything else --------- +// +// One device sent 498 `API Error: 401` events over two months. The cause was +// not an automated retry loop — it was a human re-tapping Connect because +// v0.4.4's probe scored any HTTP response as ok:true and reported +// "connection actually works now" while the password was wrong. `requireOk` +// stopped that lie; these lock in the follow-up: 401/403 is its own, +// actionable classification, not the generic health-failed bucket. + +test("health probe 401 -> auth-failed, and the summary tells the user to fix credentials", () => { + const r = classify( + okUrl, + probe({ ok: false, error: "HTTP 401", status: 401 }), // health + probe({ ok: true }), // internet + probe({ ok: true, status: 401 }), // root reachable + ) + assert.equal(r.classification, "auth-failed") + assert.match(r.summary, /401/) + assert.match(r.summary, /credential|password/i) + // Must NOT tell the user the connection works (the v0.4.4 defect). + assert.doesNotMatch(r.summary, /actually works/i) +}) + +test("health probe 403 -> auth-failed too", () => { + const r = classify(okUrl, probe({ ok: false, status: 403 }), probe({ ok: true }), probe({ ok: true, status: 403 })) + assert.equal(r.classification, "auth-failed") + assert.match(r.summary, /403/) +}) + +test("auth-failed wins even when the server root probe is unreachable", () => { + // A 401 from /global/health already proves the server answered us; the + // root probe's outcome must not downgrade this to server-unreachable. + const r = classify(okUrl, probe({ ok: false, status: 401 }), probe({ ok: true }), probe({ ok: false })) + assert.equal(r.classification, "auth-failed") +}) + +test("non-auth failure statuses stay health-failed", () => { + for (const status of [404, 500, 502]) { + const r = classify(okUrl, probe({ ok: false, status }), probe({ ok: true }), probe({ ok: true, status })) + assert.equal(r.classification, "health-failed", `status ${status}`) + } +}) + +test("health-failed copy no longer blames auth now that auth-failed exists", () => { + const r = classify(okUrl, probe({ ok: false, status: 500 }), probe({ ok: true }), probe({ ok: true, status: 500 })) + assert.doesNotMatch(r.summary, /auth/i) +}) + test("server-unreachable adds MagicDNS hint only for hostnames, not IPs", () => { const fail = { internet: probe({ ok: true }), root: probe({ ok: false }) } const hostR = classify(parseUrl("http://box.ts.net:8080"), probe({ error: "refused" }), fail.internet, fail.root) diff --git a/src/lib/diagnostics-classify.ts b/src/lib/diagnostics-classify.ts index cdd8082b..ff1be6d3 100644 --- a/src/lib/diagnostics-classify.ts +++ b/src/lib/diagnostics-classify.ts @@ -7,6 +7,7 @@ export type Classification = | "malformed-url" | "no-internet" | "server-unreachable" + | "auth-failed" | "health-failed" | "tls-error" | "timeout" @@ -55,6 +56,27 @@ export function classify( return { classification: "ok", summary: "Health endpoint responded — connection actually works now." } } + // 401/403 means the server is up, reachable, and talking to us — it just + // rejected our credentials. That needs the opposite instruction from every + // other failure below (fix the password, not the network), so it gets its + // own classification rather than being folded into the generic + // "health-failed / likely wrong path, auth, or an old server version". + // + // This is the tail of AGE-107: one device produced 498 `API Error: 401` + // events over two months because v0.4.4's probe counted any HTTP response + // as ok:true and told the user "connection actually works now" while the + // password was wrong. `requireOk` (v0.4.8) stopped the lie; this makes the + // remaining message actionable so the user stops retrying blind. + if (health.status === 401 || health.status === 403) { + return { + classification: "auth-failed", + summary: + `The server rejected your credentials (HTTP ${health.status}). ` + + `Check the password, and the username if you set OPENCODE_SERVER_USERNAME on the server ` + + `(it defaults to "opencode"). The server itself is running and reachable.`, + } + } + const txt = `${health.error ?? ""} ${health.errorCause ?? ""}`.toLowerCase() const isTls = /ssl|tls|certificate|trust|handshake/.test(txt) const isTimeout = /timeout|timed out/.test(txt) @@ -67,7 +89,7 @@ export function classify( // probe (captive portal, no WAN but Tailscale LAN still up, etc.) must not // override it and misreport a reachable server as "no internet". if (root.ok) { - return { classification: "health-failed", summary: `Server is reachable but /global/health failed (HTTP ${health.status ?? "error"}). Likely wrong path, auth, or an old server version.` } + return { classification: "health-failed", summary: `Server is reachable but /global/health failed (HTTP ${health.status ?? "error"}). Likely a wrong path or an old server version.` } } if (!internet.ok) { return { classification: "no-internet", summary: "The device has no working internet/network at all (public check also failed). Check Wi-Fi/data and Tailscale (VPN) status." } diff --git a/src/lib/sentry-noise.test.ts b/src/lib/sentry-noise.test.ts index e88b96f1..a5f5eaec 100644 --- a/src/lib/sentry-noise.test.ts +++ b/src/lib/sentry-noise.test.ts @@ -34,6 +34,16 @@ test("isTransportNoise: server-side and app-side failures are NOT noise", () => assert.equal(isTransportNoise(""), false) }) +test("isTransportNoise: AGE-107 — a rejected credential is user config, not an app defect", () => { + // The server answered 401/403. Nothing on our side can fix a wrong password; + // it is already shown in the connection screen and already trended in + // PostHog as connection_failed{error_class:"unauthorized"}. + assert.equal(isTransportNoise("connect auth-failed"), true) + // ...but a genuine 401 from anywhere else still reports — only the + // classified diagnostic is dropped. + assert.equal(isTransportNoise("API Error: 401 - "), false) +}) + test("isAlwaysSend: genuine crash classes bypass the gate", () => { assert.equal(isAlwaysSend("OutOfMemoryError (okio.Buffer.readByteArray)"), true) assert.equal(isAlwaysSend("IllegalStateException: no activity"), true) diff --git a/src/lib/sentry-noise.ts b/src/lib/sentry-noise.ts index 85eb67ea..cccdc227 100644 --- a/src/lib/sentry-noise.ts +++ b/src/lib/sentry-noise.ts @@ -15,8 +15,15 @@ // `connection_failed` event with an `error_class` property // (see analytics-classify.ts + stores/connections.ts), so dropping them from // Sentry loses no trend visibility — it just stops paying per-event for a graph -// we already have. The 401 storm is one device's token-refresh retry loop: -// 498 copies of one problem, not 498 problems. +// we already have. +// +// The 401 storm was NOT an automated retry loop (AGE-107 traced it): it was one +// user manually re-tapping Connect for two months because v0.4.4's probe scored +// any HTTP response as a success and told them "connection actually works now" +// while their password was wrong. A wrong password is user config, not an app +// defect — it is already surfaced in the connection screen (now as +// `connect auth-failed`) and already trended in PostHog as +// `connection_failed{error_class:"unauthorized"}`, so it is dropped here too. // // Three layers, cheapest first (`admit()` applies them in order): // 1. ALWAYS-SEND allowlist — genuine crash classes (OOM/ANR/native/fatal) @@ -41,12 +48,15 @@ export type NoiseEventLike = { exception?: { values?: Array<{ type?: string; value?: string; mechanism?: { handled?: boolean } }> } } -/** Client-side network conditions. Unactionable server-side, already surfaced to - * the user as connection UI, and already trended in PostHog as - * `connection_failed{error_class}`. Dropped outright. */ +/** Client-side network and credential conditions. Unactionable server-side, + * already surfaced to the user as connection UI, and already trended in + * PostHog as `connection_failed{error_class}`. Dropped outright. */ export const TRANSPORT_NOISE_PATTERNS: RegExp[] = [ // captureDiagnostic() → new Error(`connect ${classification}`) - /^connect (?:timeout|server-unreachable|no-internet|malformed-url)$/i, + // `auth-failed` = the server answered 401/403: wrong password, user config. + // `health-failed` and `tls-error` are deliberately NOT here — a box that + // answers but is unhealthy, or a broken cert, is actionable. + /^connect (?:timeout|server-unreachable|no-internet|malformed-url|auth-failed)$/i, // RN fetch failures surfacing through the global handler / rejection hook. /network request failed/i, /request timed out after/i, diff --git a/src/lib/sentry.ts b/src/lib/sentry.ts index e27c39cc..bb3c218b 100644 --- a/src/lib/sentry.ts +++ b/src/lib/sentry.ts @@ -267,11 +267,12 @@ export function captureException( export function captureDiagnostic(report: DiagnosticReport) { log.info("sentry", "capture", report.classification, enabled ? "(uploading)" : "(local only)") if (!enabled) return - // Client-side network conditions (timeout / unreachable / no internet) are - // already shown to the user and already trended in PostHog as - // `connection_failed{error_class}`; they were the single largest consumer of - // the org Sentry quota (AGE-105). Skip them here so we don't even build the - // event. `health-failed` / `tls-error` are genuinely actionable and still go. + // Client-side network conditions (timeout / unreachable / no internet) and + // wrong credentials (auth-failed) are already shown to the user and already + // trended in PostHog as `connection_failed{error_class}`; they were the + // single largest consumer of the org Sentry quota (AGE-105 / AGE-107). Skip + // them here so we don't even build the event. `health-failed` / `tls-error` + // are genuinely actionable and still go. if (isTransportNoise(`connect ${report.classification}`)) { log.info("sentry", "skipped diagnostic upload (client-side network condition)", report.classification) return