diff --git a/backend/src/controllers/imap.helpers.ts b/backend/src/controllers/imap.helpers.ts index 4386122e3..40afdde48 100644 --- a/backend/src/controllers/imap.helpers.ts +++ b/backend/src/controllers/imap.helpers.ts @@ -101,12 +101,14 @@ export function generateErrorObjectFromImapError(error: any) { errorMessage.status, errorMessage.fields ) - : new ImapAuthError(error.message, 500, [ - 'host', - 'port', - 'username', - 'password' - ]); + : new ImapAuthError( + error.message, + // Preserve a caller-supplied status. An OAuth re-auth failure arrives + // here as 401 (see MiningSourceService.getSourcesForUser); flattening + // it to 500 hid the re-auth signal from callers like passive-mining. + error.status ?? 500, + ['host', 'port', 'username', 'password'] + ); } /** diff --git a/backend/test/unit/imapHelpers.test.ts b/backend/test/unit/imapHelpers.test.ts index f0d046d7c..72ad426cc 100644 --- a/backend/test/unit/imapHelpers.test.ts +++ b/backend/test/unit/imapHelpers.test.ts @@ -91,6 +91,20 @@ describe('generateErrorObjectFromImapError', () => { } ); + it('preserves a caller-supplied status instead of defaulting to 500', () => { + const reauthError = Object.assign( + new Error( + 'Failed to fetch mining sources: Edge Function returned a non-2xx status code' + ), + { status: 401 } + ); + + const result = generateErrorObjectFromImapError(reauthError); + + expect(result).toBeInstanceOf(ImapAuthError); + expect(result.status).toEqual(401); + }); + it('should not throw when error.response is an object (AxiosError)', () => { const axiosError = { message: 'Request failed with status code 500', diff --git a/supabase/functions/_shared/mining-source-config.ts b/supabase/functions/_shared/mining-source-config.ts index 3522ed117..8a72834fd 100644 --- a/supabase/functions/_shared/mining-source-config.ts +++ b/supabase/functions/_shared/mining-source-config.ts @@ -28,6 +28,9 @@ export const SOURCE_HEALTH_SCHEMA = z.object({ state: z.nativeEnum(SourceHealthState).optional(), last_error: z.array(z.string()).nullable().optional(), last_run_at: z.string().nullable().optional(), + // Set once the user has been emailed that the source needs reconnecting, so + // the runner does not repeat the email on every run. + notified_at: z.string().nullable().optional(), }); export type SourceHealth = z.infer; diff --git a/supabase/functions/mining-sources/config.test.ts b/supabase/functions/mining-sources/config.test.ts index bba8b1617..ff58a85d2 100644 --- a/supabase/functions/mining-sources/config.test.ts +++ b/supabase/functions/mining-sources/config.test.ts @@ -1,5 +1,6 @@ import { assert, assertEquals } from "https://deno.land/std@0.224.0/assert/mod.ts"; import { mergeConfig } from "./config.ts"; +import { configureSourceSchema } from "./schemas.ts"; Deno.test("mergeConfig merges mining_flags and preserves unknown keys", () => { const result = mergeConfig( @@ -41,6 +42,23 @@ Deno.test("mergeConfig merges health fields", () => { }); }); +Deno.test("mergeConfig keeps notified_at alongside a state update", () => { + const result = mergeConfig( + { version: 1, health: { notified_at: "t1" } }, + { health: { state: "needs_reauth" } }, + ); + assertEquals(result.health, { notified_at: "t1", state: "needs_reauth" }); +}); + +Deno.test("configureSourceSchema accepts the notified_at marker", () => { + const parsed = configureSourceSchema.safeParse({ + health: { state: "needs_reauth", notified_at: "2026-10-23T02:00:00.000Z" }, + }); + + assert(parsed.success); + assertEquals(parsed.data.health?.notified_at, "2026-10-23T02:00:00.000Z"); +}); + Deno.test("mergeConfig applies monotonic per-folder cursors", () => { const current = { version: 1, diff --git a/supabase/functions/mining-sources/config.ts b/supabase/functions/mining-sources/config.ts index 9b86bf474..2eae2f3a7 100644 --- a/supabase/functions/mining-sources/config.ts +++ b/supabase/functions/mining-sources/config.ts @@ -33,6 +33,7 @@ export interface ConfigureSourceParams { state?: SourceHealthState; last_error?: string[] | null; last_run_at?: string | null; + notified_at?: string | null; }; /** Internal (service role): completion watermark. */ mining?: { diff --git a/supabase/functions/passive-mining/credential-failure.test.ts b/supabase/functions/passive-mining/credential-failure.test.ts index a4f6f073b..88472d262 100644 --- a/supabase/functions/passive-mining/credential-failure.test.ts +++ b/supabase/functions/passive-mining/credential-failure.test.ts @@ -1,89 +1,39 @@ import { assertEquals } from "https://deno.land/std@0.224.0/assert/mod.ts"; import { SourceHealthState } from "../_shared/enums.ts"; import { - isCredentialFailure, - shouldNotifyCredentialFailure, + isReauthFailure, + shouldNotifyStuckReauth, } from "./credential-failure.ts"; -Deno.test("classifies permanent OAuth errors as credential failures", () => { - assertEquals( - isCredentialFailure(new Error("invalid_grant"), "google"), - true, - ); -}); - -Deno.test("classifies OAuth re-auth responses as credential failures", () => { - assertEquals( - isCredentialFailure( - { status: 401, message: "OAuth connection needs re-authentication" }, - "google", - ), - true, - ); - assertEquals( - isCredentialFailure( - { status: 401, message: "upstream unauthorized" }, - "google", - ), - false, - ); -}); - -Deno.test("classifies IMAP HTTP 401 and 403 as credential failures", () => { - assertEquals(isCredentialFailure({ status: 401 }, "imap"), true); - assertEquals(isCredentialFailure({ status: 403 }, "imap"), true); +Deno.test("401 and 403 are reauth failures", () => { + assertEquals(isReauthFailure({ status: 401 }), true); + assertEquals(isReauthFailure({ status: 403 }), true); }); -Deno.test("does not classify transient failures as credential failures", () => { - assertEquals(isCredentialFailure({ status: 500 }, "imap"), false); - assertEquals( - isCredentialFailure(new Error("network timeout"), "imap"), - false, - ); +Deno.test("other statuses are not reauth failures", () => { + assertEquals(isReauthFailure({ status: 500 }), false); + assertEquals(isReauthFailure({ status: 400 }), false); + assertEquals(isReauthFailure(), false); + assertEquals(isReauthFailure(null), false); + assertEquals(isReauthFailure(new Error("network timeout")), false); }); -Deno.test("notifies only after a new actionable failure is persisted", () => { +Deno.test("a flagged source is notified once", () => { assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.Active, - credentialFailure: true, - healthPersisted: true, - }), + shouldNotifyStuckReauth(SourceHealthState.NeedsReauth), true, ); assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.Error, - credentialFailure: true, - healthPersisted: true, - }), - true, + shouldNotifyStuckReauth( + SourceHealthState.NeedsReauth, + "2026-10-23T02:00:00.000Z", + ), + false, ); }); -Deno.test("does not notify for an existing or failed state transition", () => { - assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.NeedsReauth, - credentialFailure: true, - healthPersisted: true, - }), - false, - ); - assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.Active, - credentialFailure: false, - healthPersisted: true, - }), - false, - ); - assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.Active, - credentialFailure: true, - healthPersisted: false, - }), - false, - ); +Deno.test("only flagged sources use the stuck-notification path", () => { + assertEquals(shouldNotifyStuckReauth(SourceHealthState.Active), false); + assertEquals(shouldNotifyStuckReauth(SourceHealthState.Error), false); + assertEquals(shouldNotifyStuckReauth(), false); }); diff --git a/supabase/functions/passive-mining/credential-failure.ts b/supabase/functions/passive-mining/credential-failure.ts index 67da47f5b..7337235c3 100644 --- a/supabase/functions/passive-mining/credential-failure.ts +++ b/supabase/functions/passive-mining/credential-failure.ts @@ -1,40 +1,25 @@ -import { isPermanentOAuthError } from "../fetch-mining-source/oauth-handler/index.ts"; import { SourceHealthState } from "../_shared/enums.ts"; -type NotificationState = SourceHealthState | undefined; - -export function isCredentialFailure( - error: unknown, - sourceType?: string, -): boolean { - const status = typeof error === "object" && error !== null - ? (error as { status?: unknown }).status - : undefined; - const isAuthStatus = status === 401 || status === 403; - - if (sourceType === "imap") return isAuthStatus; - if (sourceType !== "google" && sourceType !== "azure") return false; - if (isPermanentOAuthError(error)) return true; - - const message = typeof error === "object" && error !== null - ? (error as { message?: unknown }).message - : undefined; - return isAuthStatus && typeof message === "string" && - /re-?auth|reconnect/i.test(message); +/** + * A 401/403 from the backend means the credentials are dead and the user has + * to reconnect. Any other status is an ordinary run failure. + */ +export function isReauthFailure(error?: unknown): boolean { + const status = + typeof error === "object" && error !== null + ? (error as { status?: unknown }).status + : undefined; + return status === 401 || status === 403; } -export function shouldNotifyCredentialFailure({ - previousState, - credentialFailure, - healthPersisted, -}: { - previousState: NotificationState; - credentialFailure: boolean; - healthPersisted: boolean; -}): boolean { - return ( - previousState !== SourceHealthState.NeedsReauth && - credentialFailure && - healthPersisted - ); +/** + * True when a source is already flagged for re-auth but has not been emailed + * about it yet. The runner skips flagged sources, so this is the only chance + * to tell the user about one that was flagged before they could be notified. + */ +export function shouldNotifyStuckReauth( + state?: unknown, + notifiedAt?: unknown, +): boolean { + return state === SourceHealthState.NeedsReauth && !notifiedAt; } diff --git a/supabase/functions/passive-mining/index.ts b/supabase/functions/passive-mining/index.ts index 5e82da216..8fd959137 100644 --- a/supabase/functions/passive-mining/index.ts +++ b/supabase/functions/passive-mining/index.ts @@ -2,8 +2,8 @@ import { Context, Hono } from "npm:hono@4.7.4"; import { createSupabaseAdmin } from "../_shared/supabase.ts"; import { getFolders } from "./boxes.ts"; import { - isCredentialFailure, - shouldNotifyCredentialFailure, + isReauthFailure, + shouldNotifyStuckReauth, } from "./credential-failure.ts"; import { type MiningSourceConfigV1, @@ -98,6 +98,17 @@ async function notifyCredentialFailure( } } +/** + * Emails the user and records that we did, so a source left flagged for + * re-auth is not reported again on every run. + */ +async function notifyReauthOnce(miningSource: MiningSource): Promise { + await notifyCredentialFailure(miningSource.user_id, miningSource.email); + await patchSourceConfig(miningSource.id, { + health: { notified_at: new Date().toISOString() }, + }); +} + // Bound backend error text: validation failures echo request input, so never // propagate unbounded response bodies into logs or stored run history. const MAX_ERROR_DETAIL_LENGTH = 300; @@ -139,6 +150,17 @@ app.post("/", async (c: Context) => { const miningSources = await getMiningSources(); console.log(`Found ${miningSources.length} mining sources`); for (const miningSource of miningSources) { + const health = miningSource.parsedConfig?.health; + + // A source already flagged for re-auth cannot be mined. Tell the user + // once — these are skipped for good, so this is the only chance. + if (health?.state === SourceHealthState.NeedsReauth) { + if (shouldNotifyStuckReauth(health.state, health.notified_at)) { + await notifyReauthOnce(miningSource); + } + continue; + } + try { await recordRunStart(miningSource.id); await startMiningEmail(miningSource); @@ -153,26 +175,10 @@ app.post("/", async (c: Context) => { `Error starting mining for source ${miningSource.id}:`, errorMessage, ); - const credentialFailure = isCredentialFailure( - error, - miningSource.type, - ); - const healthTransition = await recordRunFailure( - miningSource.id, - errorMessage, - credentialFailure, - ); - if ( - shouldNotifyCredentialFailure({ - previousState: healthTransition?.previousHealthState, - credentialFailure, - healthPersisted: healthTransition !== null, - }) - ) { - await notifyCredentialFailure( - miningSource.user_id, - miningSource.email, - ); + const needsReauth = isReauthFailure(error); + await recordRunFailure(miningSource.id, errorMessage, needsReauth); + if (needsReauth) { + await notifyReauthOnce(miningSource); } } } @@ -190,14 +196,12 @@ app.post("/", async (c: Context) => { Deno.serve((req) => app.fetch(req)); async function getMiningSources() { - // Sources enabled for continuous mining that aren't awaiting re-auth. + // Every passive source, including ones already flagged for re-auth. The + // flagged ones are not mined (see the run loop) but they still need their + // one-time "reconnect" email. `type` is used for the reauth classification. // - // We fetch broadly (any passive source) and do the re-auth filter in code: - // PostgREST jsonb filters are fragile here while config migrates from the - // legacy `needs_reauth` shape to `health.state` (a `not.eq` on an absent - // key evaluates to NULL and drops valid rows). The in-code filter below - // covers both shapes. `type` is needed for the OAuth-401 classification - // (#2880) in the error path. + // Filtering is done in code rather than in PostgREST: a jsonb `not.eq` on an + // absent key evaluates to NULL and silently drops valid rows. const { data, error } = await supabase .schema("private") .from("mining_sources") @@ -209,22 +213,10 @@ async function getMiningSources() { throw error; } - return (data ?? []) - .filter((source) => { - const config = parseConfig(source.config); - const healthState = config.health?.state; - // Legacy fallback: an explicit needs_reauth:true (old shape) also skips. - const legacyNeedsReauth = - (source.config as Record | undefined)?.needs_reauth === - true; - return ( - healthState !== SourceHealthState.NeedsReauth && !legacyNeedsReauth - ); - }) - .map((source) => ({ - ...source, - parsedConfig: parseConfig(source.config), - })); + return (data ?? []).map((source) => ({ + ...source, + parsedConfig: parseConfig(source.config), + })); } async function getBoxes(miningSource: MiningSource) {