From d8364e5c9688ca8a0a07cdde578da8e16532bfaf Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Tue, 28 Jul 2026 10:44:02 -0400 Subject: [PATCH] fix(poll): trigger re-auth on 401 during hot-poll instead of retrying The hot-poll cycle (PR status + workflow run refresh) absorbed 401 Bad Credentials errors into the generic hadErrors/backoff path, so an expired or revoked token produced an endless "retrying with backoff" loop instead of prompting re-authentication. The full poll already special-cases 401 to trigger expireToken() + redirect; hot-poll now does the same via a shared isUnauthorizedError() helper. Fixes GITHUB-TRACKER-6 --- src/app/services/api.ts | 8 ++ src/app/services/poll.ts | 16 ++- tests/services/hot-poll.test.ts | 121 +++++++++++++++++++++++ tests/services/poll-fetchAllData.test.ts | 5 + 4 files changed, 148 insertions(+), 2 deletions(-) diff --git a/src/app/services/api.ts b/src/app/services/api.ts index 517e6a5f..8879f380 100644 --- a/src/app/services/api.ts +++ b/src/app/services/api.ts @@ -84,6 +84,12 @@ export function resetEmptyActionRepos(): void { * Normalizes a Promise.allSettled rejection reason into a structured error shape. * Handles both Octokit RequestError (has `.status`) and plain Error objects. */ +export function isUnauthorizedError(err: unknown): boolean { + if (typeof err !== "object" || err === null) return false; + const e = err as { status?: unknown; response?: { status?: unknown } }; + return e.status === 401 || e.response?.status === 401; +} + function extractRejectionError(reason: unknown): { statusCode: number | null; message: string } { const statusCode = typeof reason === "object" && @@ -1804,6 +1810,8 @@ export async function fetchHotPRStatus( for (const s of settled) { if (s.status === "rejected") { + // Token is invalid — let the caller trigger re-auth instead of retrying forever. + if (isUnauthorizedError(s.reason)) throw s.reason; hadErrors = true; console.warn("[hot-poll] PR status batch failed:", s.reason); Sentry.captureException(s.reason, { tags: { source: "hot-poll-pr-batch" } }); diff --git a/src/app/services/poll.ts b/src/app/services/poll.ts index 0758e903..ba7b7520 100644 --- a/src/app/services/poll.ts +++ b/src/app/services/poll.ts @@ -2,7 +2,7 @@ import { createSignal, createEffect, createRoot, untrack, onCleanup } from "soli import * as Sentry from "@sentry/solid"; import { getClient, fetchRateLimitDetails } from "./github"; import { config } from "../stores/config"; -import { user, onAuthCleared } from "../stores/auth"; +import { user, onAuthCleared, expireToken } from "../stores/auth"; import { checkAndResetIfExpired } from "./api-usage"; import { fetchIssuesAndPullRequests, @@ -10,6 +10,7 @@ import { fetchHotPRStatus, fetchWorkflowRunById, pooledAllSettled, + isUnauthorizedError, type Issue, type PullRequest, type WorkflowRun, @@ -228,7 +229,7 @@ export async function fetchAllData( if (result.status === "rejected") { const err = result.reason; // Propagate 401 to outer handler for re-auth (don't absorb as generic error) - if (err?.status === 401 || err?.response?.status === 401) throw err; + if (isUnauthorizedError(err)) throw err; const statusCode = typeof err === "object" && err !== null && typeof (err as Record).status === "number" ? (err as Record).status as number : null; @@ -493,6 +494,7 @@ export async function fetchHotData(): Promise<{ prUpdates.set(id, update); } } catch (err) { + if (isUnauthorizedError(err)) throw err; hadErrors = true; console.warn("[hot-poll] PR status fetch failed:", err); Sentry.captureException(err, { tags: { source: "hot-poll-pr-fetch" } }); @@ -509,6 +511,7 @@ export async function fetchHotData(): Promise<{ if (result.status === "fulfilled") { runUpdates.set(result.value.id, result.value); } else { + if (isUnauthorizedError(result.reason)) throw result.reason; hadErrors = true; console.warn("[hot-poll] Workflow run fetch failed:", result.reason); Sentry.captureException(result.reason, { tags: { source: "hot-poll-run-fetch" } }); @@ -639,6 +642,15 @@ export function createHotPollCoordinator( onHotData(prUpdates, runUpdates, generation); } } catch (err) { + if (isUnauthorizedError(err)) { + // Token invalid — same re-auth path as the full poll (poll.ts throw at + // fetchAllData) and cross-tab sync (auth.ts storage listener): clear the + // token and hard-redirect. Don't schedule another cycle — page is navigating. + Sentry.captureException(err, { tags: { source: "hot-poll-auth-expired" } }); + expireToken(); + window.location.replace("/login"); + return; + } consecutiveFailures++; const message = err instanceof Error ? err.message : "Unknown hot-poll error"; pushError("hot-poll", message, true); diff --git a/tests/services/hot-poll.test.ts b/tests/services/hot-poll.test.ts index 17416060..b3c714f6 100644 --- a/tests/services/hot-poll.test.ts +++ b/tests/services/hot-poll.test.ts @@ -4,6 +4,16 @@ import { makePullRequest, makeWorkflowRun } from "../helpers/index"; // ── Mocks ───────────────────────────────────────────────────────────────────── +const mockLocationReplace = vi.fn(); +// createHotPollCoordinator hard-redirects to /login on 401, mirroring the +// full-poll and cross-tab-sync auth paths. Stub window.location so we can +// assert on the redirect without actually navigating. +Object.defineProperty(window, "location", { + configurable: true, + writable: true, + value: { replace: mockLocationReplace, href: "" }, +}); + // Mock github module so getClient returns our fake octokit const mockGetClient = vi.fn(); vi.mock("../../src/app/services/github", () => ({ @@ -49,6 +59,7 @@ vi.mock("../../src/app/stores/config", () => ({ vi.mock("../../src/app/stores/auth", () => ({ user: vi.fn(() => null), onAuthCleared: vi.fn(), + expireToken: vi.fn(), })); // Import AFTER mocks are set up @@ -65,6 +76,7 @@ import { import { fetchHotPRStatus, fetchWorkflowRunById, + isUnauthorizedError, } from "../../src/app/services/api"; // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -89,6 +101,28 @@ const emptyData: DashboardData = { // ── Tests ───────────────────────────────────────────────────────────────────── +describe("isUnauthorizedError", () => { + it("returns true for a top-level status 401", () => { + expect(isUnauthorizedError({ status: 401 })).toBe(true); + }); + + it("returns true for a nested response.status 401", () => { + expect(isUnauthorizedError({ response: { status: 401 } })).toBe(true); + }); + + it("returns false for other status codes", () => { + expect(isUnauthorizedError({ status: 500 })).toBe(false); + expect(isUnauthorizedError({ response: { status: 403 } })).toBe(false); + }); + + it("returns false for non-object and null values", () => { + expect(isUnauthorizedError(null)).toBe(false); + expect(isUnauthorizedError(undefined)).toBe(false); + expect(isUnauthorizedError("Bad credentials")).toBe(false); + expect(isUnauthorizedError(new Error("network failure"))).toBe(false); + }); +}); + describe("fetchHotPRStatus", () => { it("returns empty map for empty nodeIds", async () => { const octokit = makeOctokit(); @@ -800,6 +834,60 @@ describe("createHotPollCoordinator", () => { }); }); + it("redirects to /login via expireToken on 401 instead of generic backoff", async () => { + const onHotData = vi.fn(); + const octokit = makeOctokit(undefined, () => + Promise.reject(Object.assign(new Error("Bad credentials"), { status: 401 }))); + mockGetClient.mockReturnValue(octokit); + + rebuildHotSets({ + ...emptyData, + pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })], + }); + + const { expireToken } = await import("../../src/app/stores/auth"); + const { pushError } = await import("../../src/app/lib/errors"); + (expireToken as ReturnType).mockClear(); + (pushError as ReturnType).mockClear(); + mockLocationReplace.mockClear(); + + await createRoot(async (dispose) => { + createHotPollCoordinator(() => 10, onHotData); + await vi.advanceTimersByTimeAsync(10_000); + + expect(expireToken).toHaveBeenCalledOnce(); + expect(mockLocationReplace).toHaveBeenCalledWith("/login"); + // Not treated as a generic retryable failure + expect(pushError).not.toHaveBeenCalledWith("hot-poll", expect.any(String), true); + dispose(); + }); + }); + + it("does not schedule another cycle after a 401 redirect", async () => { + const onHotData = vi.fn(); + const octokit = makeOctokit(undefined, () => + Promise.reject(Object.assign(new Error("Bad credentials"), { status: 401 }))); + mockGetClient.mockReturnValue(octokit); + + rebuildHotSets({ + ...emptyData, + pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })], + }); + + const graphqlFn = octokit.graphql as ReturnType; + + await createRoot(async (dispose) => { + createHotPollCoordinator(() => 10, onHotData); + await vi.advanceTimersByTimeAsync(10_000); + expect(graphqlFn).toHaveBeenCalledTimes(1); + + // Advance well past several would-be cycles — no further fetch attempts + await vi.advanceTimersByTimeAsync(60_000); + expect(graphqlFn).toHaveBeenCalledTimes(1); + dispose(); + }); + }); + it("does not schedule when interval is 0", async () => { const onHotData = vi.fn(); mockGetClient.mockReturnValue(makeOctokit()); @@ -898,6 +986,13 @@ describe("fetchHotPRStatus edge cases", () => { expect(results.get(1)).toBeDefined(); expect(hadErrors).toBe(true); }); + + it("throws (does not swallow into hadErrors) when a batch rejects with 401", async () => { + const octokit = makeOctokit(undefined, () => + Promise.reject(Object.assign(new Error("Bad credentials"), { status: 401 }))); + + await expect(fetchHotPRStatus(octokit as never, ["PR_a"])).rejects.toMatchObject({ status: 401 }); + }); }); describe("rebuildHotSets caps", () => { @@ -1137,6 +1232,32 @@ describe("fetchHotData hadErrors", () => { expect(hadErrors).toBe(true); expect(runUpdates.size).toBe(0); // failed, no results }); + + it("propagates (throws) a 401 from the PR fetch instead of returning hadErrors", async () => { + const octokit = makeOctokit(undefined, () => + Promise.reject(Object.assign(new Error("Bad credentials"), { status: 401 }))); + mockGetClient.mockReturnValue(octokit); + + rebuildHotSets({ + ...emptyData, + pullRequests: [makePullRequest({ id: 1, checkStatus: "pending", enriched: true, nodeId: "PR_a" })], + }); + + await expect(fetchHotData()).rejects.toMatchObject({ status: 401 }); + }); + + it("propagates (throws) a 401 from a workflow run fetch instead of returning hadErrors", async () => { + const octokit = makeOctokit(() => + Promise.reject(Object.assign(new Error("Bad credentials"), { status: 401 }))); + mockGetClient.mockReturnValue(octokit); + + rebuildHotSets({ + ...emptyData, + workflowRuns: [makeWorkflowRun({ id: 10, status: "in_progress", conclusion: null, repoFullName: "o/r" })], + }); + + await expect(fetchHotData()).rejects.toMatchObject({ status: 401 }); + }); }); describe("fetchHotPRStatus updateGraphqlRateLimit", () => { diff --git a/tests/services/poll-fetchAllData.test.ts b/tests/services/poll-fetchAllData.test.ts index 42d79927..183df0bd 100644 --- a/tests/services/poll-fetchAllData.test.ts +++ b/tests/services/poll-fetchAllData.test.ts @@ -30,6 +30,11 @@ vi.mock("../../src/app/services/api", () => ({ fetchIssuesAndPullRequests: vi.fn(), fetchWorkflowRuns: vi.fn(), resetEmptyActionRepos: vi.fn(), + isUnauthorizedError: (err: unknown) => { + if (typeof err !== "object" || err === null) return false; + const e = err as { status?: unknown; response?: { status?: unknown } }; + return e.status === 401 || e.response?.status === 401; + }, })); // Mock notifications