From 3b6dab8c2788c727c694918e9882da01ba0d180a Mon Sep 17 00:00:00 2001 From: Den <2119348+dzianisv@users.noreply.github.com> Date: Fri, 17 Jul 2026 02:22:53 -0700 Subject: [PATCH] fix(auth): stop infinite SSE retry on 401/403, surface auth failures (#79) * fix(auth): stop infinite SSE retry on 401/403 and surface auth failures Root cause (Sentry OPENCODE-MOBILE-1, 309 events / 65 users): auth is static HTTP Basic and no code path treated 401 specially. The SSE reconnect loop in events.ts retried on a fixed backoff regardless of cause, so a bad password spammed Sentry and drained battery forever with zero user feedback. Advanced-mode connection save also had no pre-flight check and silently persisted bad credentials as the active connection. - src/lib/api-error.ts: new pure ApiAuthError/isAuthStatus/isAuthError module (node --test covered) so 401/403 are distinguishable from other failures. - src/lib/sdk.ts: request()/events() now throw ApiAuthError for 401/403 instead of a generic Error. - src/stores/events.ts: the SSE loop stops retrying on an auth error and sets a new `authError` flag instead of reconnecting forever; other errors keep the existing backoff. Fires connection_failed (source: sse, error_class: unauthorized) so it's visible in the existing funnel. - app/(tabs)/index.tsx: sessions screen shows an "Authentication Failed" state with a link to the connection edit screen when authError is set. - app/connection/[id].tsx: saving edited credentials for the active connection now reconnects SSE immediately instead of requiring an app restart. - app/connection/add.tsx: Advanced-mode save now runs the same testConnection pre-flight as Quick Connect and shows the same "Connection Failed" alert (with diagnostics/share-report) instead of silently saving bad credentials. Closes #76 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NJKAQ6HAikWGQK7PGZ5Y4E * fix(auth): add Retry button on 401 error state, widen ConnectionTestSource - Authentication Failed screen now offers Retry alongside Check Credentials, calling events store's connect() directly to restart the SSE state machine on transient 401s without leaving the app. - Widen ConnectionTestSource to include 'sse' (events.ts:389's connection_failed track call) and note the activation funnel only filters on source=onboarding. Addresses PR #79 review follow-ups. --------- Co-authored-by: engineer Co-authored-by: Claude Fable 5 --- app/(tabs)/index.tsx | 44 +++++++++++++++++++++++++++++++++++ app/connection/[id].tsx | 8 +++++++ app/connection/add.tsx | 48 ++++++++++++++++++++++++++++++++++----- src/lib/analytics.ts | 11 +++++---- src/lib/api-error.test.ts | 42 ++++++++++++++++++++++++++++++++++ src/lib/api-error.ts | 38 +++++++++++++++++++++++++++++++ src/lib/sdk.ts | 7 ++++-- src/stores/events.ts | 31 +++++++++++++++++++++++-- 8 files changed, 214 insertions(+), 15 deletions(-) create mode 100644 src/lib/api-error.test.ts create mode 100644 src/lib/api-error.ts diff --git a/app/(tabs)/index.tsx b/app/(tabs)/index.tsx index 4bc9a71..a8bac0c 100644 --- a/app/(tabs)/index.tsx +++ b/app/(tabs)/index.tsx @@ -19,6 +19,7 @@ import { router, useFocusEffect } from "expo-router" import { Ionicons } from "@expo/vector-icons" import { useSessions } from "../../src/stores/sessions" import { useConnections } from "../../src/stores/connections" +import { useEvents } from "../../src/stores/events" import { useCatalog } from "../../src/stores/catalog" import type BottomSheet from "@gorhom/bottom-sheet" import type { Session, Project } from "../../src/lib/sdk" @@ -133,6 +134,8 @@ export default function SessionsScreen() { addRecentDirectory, recentDirectories, } = useConnections() + const authError = useEvents((s) => s.authError) + const reconnect = useEvents((s) => s.connect) const loadCatalog = useCatalog((s) => s.load) const dirSheetRef = useRef(null) const browserSheetRef = useRef(null) @@ -348,6 +351,43 @@ export default function SessionsScreen() { ) } + // The SSE loop stopped retrying because the server rejected our + // credentials (401/403) — no amount of pull-to-refresh fixes that, so + // point the user straight at the fix instead of a spinner that never + // resolves (issue #76). + if (authError) { + return ( + + + Authentication Failed + + {activeConnection.name} rejected your credentials. Check the username and password to reconnect. + + + router.push(`/connection/${activeConnection.id}`)} + testID="fix-connection-button" + > + Check Credentials + + { + // authError is cleared inside connect() itself once the retry + // attempt starts (see src/stores/events.ts), so a manual + // set() here isn't needed — just kick the SSE state machine. + reconnect() + }} + testID="retry-connection-button" + > + Retry + + + + ) + } + const shortPath = getShortPath(currentProject) return ( @@ -832,6 +872,10 @@ const styles = StyleSheet.create({ borderRadius: 8, marginTop: 24, }, + authErrorButtonRow: { + flexDirection: "row", + gap: 12, + }, addButtonDark: { backgroundColor: "#ffffff", }, diff --git a/app/connection/[id].tsx b/app/connection/[id].tsx index c873e52..79feebe 100644 --- a/app/connection/[id].tsx +++ b/app/connection/[id].tsx @@ -13,6 +13,7 @@ import { import { router, useLocalSearchParams } from "expo-router" import { Ionicons } from "@expo/vector-icons" import { useConnections } from "../../src/stores/connections" +import { useEvents } from "../../src/stores/events" import type { ConnectionType } from "../../src/lib/types" import { probeConnection, shareReport } from "../../src/lib/diagnostics" import { captureDiagnostic } from "../../src/lib/sentry" @@ -130,6 +131,13 @@ export default function EditConnectionScreen() { directory: directory.trim() || undefined, username: username.trim() || undefined, }) + // If this was the active connection, the SSE loop may have stopped + // retrying after a prior 401 (see events.ts) — reconnect now with the + // freshly saved credentials instead of leaving the user stuck until + // they relaunch the app. + if (useConnections.getState().activeConnection?.id === connection.id) { + useEvents.getState().connect() + } setIsSaving(false) router.back() } diff --git a/app/connection/add.tsx b/app/connection/add.tsx index ba53492..a92d06a 100644 --- a/app/connection/add.tsx +++ b/app/connection/add.tsx @@ -133,23 +133,59 @@ export default function AddConnectionScreen() { } track(AnalyticsEvent.ConnectionFormSubmitted, { mode: "advanced" }) - // Advanced mode saves directly without a pre-flight health check (see - // useConnections.addConnection), so unlike quick-connect there is no - // success/failure signal to report here — only that an attempt was made. - track(AnalyticsEvent.ConnectionAttempted, { source: "onboarding" }) setIsConnecting(true) - await addConnection( + + // Pre-flight, mirroring Quick Connect: previously Advanced mode saved + // directly with no health check, so bad credentials (401/403) or an + // unreachable server silently became the active connection with zero + // feedback (issue #76). testConnection() also fires the + // connection_attempted/succeeded/failed analytics events. + const result = await testConnection( { + id: "", name: name.trim(), type, url: url.trim(), directory: directory.trim() || undefined, username: username.trim() || undefined, }, + "onboarding", password || undefined, ) + + if (result.ok) { + await addConnection( + { + name: name.trim(), + type, + url: url.trim(), + directory: directory.trim() || undefined, + username: username.trim() || undefined, + }, + password || undefined, + ) + setIsConnecting(false) + router.back() + return + } + + // Failed: same "Connection Failed" alert as Quick Connect — run active + // diagnostics, capture to Sentry, and offer a shareable report instead of + // silently persisting an unreachable/unauthorized connection. + const report = await probeConnection( + url.trim(), + username.trim() && password ? { username: username.trim(), password } : undefined, + ) + captureDiagnostic(report) setIsConnecting(false) - router.back() + Alert.alert( + "Connection Failed", + `${report.summary}\n\nTarget: ${url.trim()}\nError: ${result.error || "Unknown error"}`, + [ + { text: "OK", style: "cancel" }, + { text: "Share report", onPress: () => shareReport(report) }, + ], + ) } const handleJoinWaitlist = async () => { diff --git a/src/lib/analytics.ts b/src/lib/analytics.ts index 4e4e106..966bbfe 100644 --- a/src/lib/analytics.ts +++ b/src/lib/analytics.ts @@ -87,11 +87,12 @@ export enum AnalyticsEvent { ResponseReceived = "response_received", } -/** Where a connection test was initiated from. The activation funnel filters - * to source=onboarding; edit_test covers the Test button on the existing- - * connection edit screen, which would otherwise pollute the funnel with - * repeat-tester noise. */ -export type ConnectionTestSource = "onboarding" | "edit_test" +/** Where a connection test/failure was initiated from. The activation funnel + * filters to source=onboarding only; edit_test (Test button on the + * existing-connection edit screen) and sse (background reconnect loop, + * see events.ts) would otherwise pollute the funnel with repeat-tester and + * post-activation noise. */ +export type ConnectionTestSource = "onboarding" | "edit_test" | "sse" export function initAnalytics() { if (enabled) return diff --git a/src/lib/api-error.test.ts b/src/lib/api-error.test.ts new file mode 100644 index 0000000..3806564 --- /dev/null +++ b/src/lib/api-error.test.ts @@ -0,0 +1,42 @@ +import { test } from "node:test" +import assert from "node:assert/strict" +import { ApiAuthError, apiErrorFor, isAuthError, isAuthStatus } from "./api-error.ts" + +test("isAuthStatus: 401 and 403 are auth failures", () => { + assert.equal(isAuthStatus(401), true) + assert.equal(isAuthStatus(403), true) +}) + +test("isAuthStatus: other statuses are not auth failures", () => { + assert.equal(isAuthStatus(200), false) + assert.equal(isAuthStatus(404), false) + assert.equal(isAuthStatus(500), false) + assert.equal(isAuthStatus(503), false) + assert.equal(isAuthStatus(0), false) +}) + +test("apiErrorFor: 401/403 produce an ApiAuthError carrying the status", () => { + const err401 = apiErrorFor(401, "API Error: 401 - Unauthorized") + assert.ok(err401 instanceof ApiAuthError) + assert.equal(err401.status, 401) + assert.equal(err401.message, "API Error: 401 - Unauthorized") + + const err403 = apiErrorFor(403, "API Error: 403 - Forbidden") + assert.ok(err403 instanceof ApiAuthError) + assert.equal(err403.status, 403) +}) + +test("apiErrorFor: other statuses produce a plain Error, not ApiAuthError", () => { + const err = apiErrorFor(500, "API Error: 500 - Internal Server Error") + assert.ok(err instanceof Error) + assert.equal(err instanceof ApiAuthError, false) + assert.equal(err.message, "API Error: 500 - Internal Server Error") +}) + +test("isAuthError: type guard matches only ApiAuthError instances", () => { + assert.equal(isAuthError(new ApiAuthError(401, "nope")), true) + assert.equal(isAuthError(new Error("some other error")), false) + assert.equal(isAuthError("401"), false) + assert.equal(isAuthError(undefined), false) + assert.equal(isAuthError(null), false) +}) diff --git a/src/lib/api-error.ts b/src/lib/api-error.ts new file mode 100644 index 0000000..97f1d96 --- /dev/null +++ b/src/lib/api-error.ts @@ -0,0 +1,38 @@ +// Pure classification of HTTP auth failures, extracted so it's unit-testable +// under plain `node --test` without pulling in expo/fetch (sdk.ts is RN-only) +// — same pattern as analytics-classify.ts / diagnostics-classify.ts. +// +// Why this exists: sdk.ts's request()/events() used to throw a generic Error +// for every non-2xx response, so call sites (the SSE reconnect loop, screen +// error states) had no way to tell "your password is wrong" apart from "the +// server is briefly unreachable" — a 401 got treated like any transient +// failure and retried forever (see events.ts's reconnect loop / issue #76). + +/** Thrown by sdk.ts's request()/events() when the server responds 401/403, + * so call sites can distinguish "bad credentials" from any other failure + * (network error, 5xx, timeout) instead of catching a generic Error. */ +export class ApiAuthError extends Error { + readonly status: number + constructor(status: number, message: string) { + super(message) + this.name = "ApiAuthError" + this.status = status + } +} + +/** True for HTTP statuses that mean "your credentials are wrong", as opposed + * to transient/network/server failures that should keep retrying. */ +export function isAuthStatus(status: number): boolean { + return status === 401 || status === 403 +} + +/** Build the right error type for a failed HTTP response. */ +export function apiErrorFor(status: number, message: string): Error { + return isAuthStatus(status) ? new ApiAuthError(status, message) : new Error(message) +} + +/** Type guard for call sites (e.g. the SSE reconnect loop) that need to branch + * on whether a caught error was an auth failure. */ +export function isAuthError(error: unknown): error is ApiAuthError { + return error instanceof ApiAuthError +} diff --git a/src/lib/sdk.ts b/src/lib/sdk.ts index b451269..74949e5 100644 --- a/src/lib/sdk.ts +++ b/src/lib/sdk.ts @@ -5,6 +5,9 @@ import { fetch as expoFetch } from "expo/fetch" import { buildRequestHeaders } from "./headers" import { SSEParser } from "./sse" +import { apiErrorFor } from "./api-error" + +export { ApiAuthError, isAuthError } from "./api-error" export interface ClientConfig { baseUrl: string @@ -196,7 +199,7 @@ async function request(config: ClientConfig, path: string, options: RequestIn if (!response.ok) { const error = await response.text() - throw new ApiError(response.status, error) + throw apiErrorFor(response.status, `API Error: ${response.status} - ${error}`) } return response.json() @@ -243,7 +246,7 @@ export function createClient(config: ClientConfig) { // Must use expo/fetch for ReadableStream support on native const response = await expoFetch(url, { headers, signal }) if (!response.ok || !response.body) { - throw new Error(`Failed to connect to event stream: ${response.status}`) + throw apiErrorFor(response.status, `Failed to connect to event stream: ${response.status}`) } const reader = response.body.getReader() diff --git a/src/stores/events.ts b/src/stores/events.ts index 875508c..d972d2d 100644 --- a/src/stores/events.ts +++ b/src/stores/events.ts @@ -7,6 +7,7 @@ import { statusFromPart } from "../lib/status-labels" import { addBreadcrumb } from "../lib/sentry" import { AnalyticsEvent, track } from "../lib/analytics" import { recordSuccessfulSession } from "../lib/store-review" +import { isAuthError } from "../lib/api-error" import type { Client, Part, Session, Message } from "../lib/sdk" // Session status from the server @@ -14,6 +15,13 @@ type SessionStatus = { type: "idle" } | { type: "busy" } | { type: "retry"; atte interface EventsState { connected: boolean + // Set when the last connection attempt failed with 401/403 — the server + // rejected our credentials, not a transient network issue. The reconnect + // loop stops retrying in this case (see connect()) since hammering a + // fixed-credential auth failure forever just spams Sentry/battery with no + // path to recovery (issue #76). Cleared on the next connect() attempt, + // e.g. after the user fixes their credentials on the connection edit screen. + authError: boolean reconnectAttempts: number lastDisconnectAt: number | null sessionStatus: Record @@ -82,6 +90,7 @@ export async function refreshPending(client: Client, sessionID: string) { export const useEvents = create((set, get) => ({ connected: false, + authError: false, reconnectAttempts: 0, lastDisconnectAt: null, sessionStatus: {}, @@ -102,7 +111,7 @@ export const useEvents = create((set, get) => ({ controller = new AbortController() const currentController = controller - set({ connected: true }) + set({ connected: true, authError: false }) console.log("[SSE] Connecting to event stream...") addBreadcrumb({ category: "sse", message: "connecting" }) @@ -364,7 +373,24 @@ export const useEvents = create((set, get) => ({ scheduleReconnect(new Error("Event stream closed")) } catch (err) { - scheduleReconnect(err) + if (isAuthError(err) && !currentController.signal.aborted) { + // Bad credentials, not a transient failure — retrying forever just + // spams Sentry and drains the battery with zero path to recovery + // (issue #76: 309 events / 65 users). Stop and surface a distinct + // state instead; the sessions screen offers a link to fix + // credentials, which reconnects via connect() once saved. + console.warn("[SSE] Authentication failed — stopping reconnect loop:", err) + addBreadcrumb({ + category: "sse", + level: "error", + message: "auth error - stopped retrying", + data: { status: err.status }, + }) + track(AnalyticsEvent.ConnectionFailed, { source: "sse", error_class: "unauthorized" }) + set({ connected: false, authError: true }) + } else { + scheduleReconnect(err) + } } finally { clearTimeout(stableTimer) if (currentController.signal.aborted) { @@ -387,6 +413,7 @@ export const useEvents = create((set, get) => ({ abortedSessions.clear() set({ connected: false, + authError: false, reconnectAttempts: 0, lastDisconnectAt: null, sessionStatus: {},