From a6db491f897c08a6ef95958d9b952394111ec682 Mon Sep 17 00:00:00 2001 From: Malek Salem Date: Fri, 23 Oct 2026 14:17:53 +0100 Subject: [PATCH 1/4] fix(imap): preserve the error status so a dead OAuth grant reports 401 The fallback in generateErrorObjectFromImapError hardcoded 500, which flattened the 401 raised by MiningSourceService.getSourcesForUser and hid the re-auth signal from callers such as passive-mining. --- backend/src/controllers/imap.helpers.ts | 14 ++++++++------ backend/test/unit/imapHelpers.test.ts | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) 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', From 0b9179d3f573cac7ae54b1d588c824a820364e3a Mon Sep 17 00:00:00 2001 From: Malek Salem Date: Fri, 23 Oct 2026 14:17:58 +0100 Subject: [PATCH 2/4] fix(mining): notify on a 401/403 reauth failure The notification gate depended on the health state read back from its own write. fetch-mining-source has already set needs_reauth by then, so the gate could never pass and passive-mining-failed was never invoked. Replace the two predicates with a plain status check and reuse the same boolean for the health write, so the source lands in needs_reauth and is skipped on the next run. --- .../passive-mining/credential-failure.test.ts | 94 ++----------------- .../passive-mining/credential-failure.ts | 49 ++-------- supabase/functions/passive-mining/index.ts | 24 +---- 3 files changed, 24 insertions(+), 143 deletions(-) diff --git a/supabase/functions/passive-mining/credential-failure.test.ts b/supabase/functions/passive-mining/credential-failure.test.ts index a4f6f073b..f56d5b6b8 100644 --- a/supabase/functions/passive-mining/credential-failure.test.ts +++ b/supabase/functions/passive-mining/credential-failure.test.ts @@ -1,89 +1,15 @@ import { assertEquals } from "https://deno.land/std@0.224.0/assert/mod.ts"; -import { SourceHealthState } from "../_shared/enums.ts"; -import { - isCredentialFailure, - shouldNotifyCredentialFailure, -} from "./credential-failure.ts"; +import { isReauthFailure } from "./credential-failure.ts"; -Deno.test("classifies permanent OAuth errors as credential failures", () => { - assertEquals( - isCredentialFailure(new Error("invalid_grant"), "google"), - true, - ); +Deno.test("401 and 403 are reauth failures", () => { + assertEquals(isReauthFailure({ status: 401 }), true); + assertEquals(isReauthFailure({ status: 403 }), 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("does not classify transient failures as credential failures", () => { - assertEquals(isCredentialFailure({ status: 500 }, "imap"), false); - assertEquals( - isCredentialFailure(new Error("network timeout"), "imap"), - false, - ); -}); - -Deno.test("notifies only after a new actionable failure is persisted", () => { - assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.Active, - credentialFailure: true, - healthPersisted: true, - }), - true, - ); - assertEquals( - shouldNotifyCredentialFailure({ - previousState: SourceHealthState.Error, - credentialFailure: true, - healthPersisted: true, - }), - true, - ); -}); - -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("other statuses are not reauth failures", () => { + assertEquals(isReauthFailure({ status: 500 }), false); + assertEquals(isReauthFailure({ status: 400 }), false); + assertEquals(isReauthFailure(undefined), false); + assertEquals(isReauthFailure(null), false); + assertEquals(isReauthFailure(new Error("network timeout")), false); }); diff --git a/supabase/functions/passive-mining/credential-failure.ts b/supabase/functions/passive-mining/credential-failure.ts index 67da47f5b..4e175a004 100644 --- a/supabase/functions/passive-mining/credential-failure.ts +++ b/supabase/functions/passive-mining/credential-failure.ts @@ -1,40 +1,11 @@ -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); -} - -export function shouldNotifyCredentialFailure({ - previousState, - credentialFailure, - healthPersisted, -}: { - previousState: NotificationState; - credentialFailure: boolean; - healthPersisted: boolean; -}): boolean { - return ( - previousState !== SourceHealthState.NeedsReauth && - credentialFailure && - healthPersisted - ); +/** + * 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; } diff --git a/supabase/functions/passive-mining/index.ts b/supabase/functions/passive-mining/index.ts index 5e82da216..e5fabb154 100644 --- a/supabase/functions/passive-mining/index.ts +++ b/supabase/functions/passive-mining/index.ts @@ -1,10 +1,7 @@ import { Context, Hono } from "npm:hono@4.7.4"; import { createSupabaseAdmin } from "../_shared/supabase.ts"; import { getFolders } from "./boxes.ts"; -import { - isCredentialFailure, - shouldNotifyCredentialFailure, -} from "./credential-failure.ts"; +import { isReauthFailure } from "./credential-failure.ts"; import { type MiningSourceConfigV1, parseConfig, @@ -153,22 +150,9 @@ 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, - }) - ) { + const needsReauth = isReauthFailure(error); + await recordRunFailure(miningSource.id, errorMessage, needsReauth); + if (needsReauth) { await notifyCredentialFailure( miningSource.user_id, miningSource.email, From cb81034e55730cc5cd087f19ef9ce10b14985259 Mon Sep 17 00:00:00 2001 From: Malek Salem Date: Fri, 23 Oct 2026 14:37:04 +0100 Subject: [PATCH 3/4] feat(mining): notify sources already flagged for reauth Sources in needs_reauth are skipped by the runner, so the transition-based notification can never fire for them. Include them in the run, skip mining, and email once using a health.notified_at marker so it is not repeated. --- .../functions/_shared/mining-source-config.ts | 3 + .../functions/mining-sources/config.test.ts | 18 ++++++ supabase/functions/mining-sources/config.ts | 1 + .../passive-mining/credential-failure.test.ts | 34 +++++++++- .../passive-mining/credential-failure.ts | 16 ++++- supabase/functions/passive-mining/index.ts | 64 +++++++++++-------- 6 files changed, 105 insertions(+), 31 deletions(-) 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 f56d5b6b8..b7798635e 100644 --- a/supabase/functions/passive-mining/credential-failure.test.ts +++ b/supabase/functions/passive-mining/credential-failure.test.ts @@ -1,5 +1,9 @@ import { assertEquals } from "https://deno.land/std@0.224.0/assert/mod.ts"; -import { isReauthFailure } from "./credential-failure.ts"; +import { SourceHealthState } from "../_shared/enums.ts"; +import { + isReauthFailure, + shouldNotifyStuckReauth, +} from "./credential-failure.ts"; Deno.test("401 and 403 are reauth failures", () => { assertEquals(isReauthFailure({ status: 401 }), true); @@ -9,7 +13,33 @@ Deno.test("401 and 403 are reauth failures", () => { Deno.test("other statuses are not reauth failures", () => { assertEquals(isReauthFailure({ status: 500 }), false); assertEquals(isReauthFailure({ status: 400 }), false); - assertEquals(isReauthFailure(undefined), false); + assertEquals(isReauthFailure(), false); assertEquals(isReauthFailure(null), false); assertEquals(isReauthFailure(new Error("network timeout")), false); }); + +Deno.test("a flagged source is notified once", () => { + assertEquals( + shouldNotifyStuckReauth(SourceHealthState.NeedsReauth, undefined), + true, + ); + assertEquals( + shouldNotifyStuckReauth( + SourceHealthState.NeedsReauth, + "2026-10-23T02:00:00.000Z", + ), + false, + ); +}); + +Deno.test("only flagged sources use the stuck-notification path", () => { + assertEquals( + shouldNotifyStuckReauth(SourceHealthState.Active, undefined), + false, + ); + assertEquals( + shouldNotifyStuckReauth(SourceHealthState.Error, undefined), + false, + ); + assertEquals(shouldNotifyStuckReauth(undefined, undefined), false); +}); diff --git a/supabase/functions/passive-mining/credential-failure.ts b/supabase/functions/passive-mining/credential-failure.ts index 4e175a004..647244f0e 100644 --- a/supabase/functions/passive-mining/credential-failure.ts +++ b/supabase/functions/passive-mining/credential-failure.ts @@ -1,11 +1,25 @@ +import { SourceHealthState } from "../_shared/enums.ts"; + /** * 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 { +export function isReauthFailure(error?: unknown): boolean { const status = typeof error === "object" && error !== null ? (error as { status?: unknown }).status : undefined; return status === 401 || status === 403; } + +/** + * 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 e5fabb154..8fd959137 100644 --- a/supabase/functions/passive-mining/index.ts +++ b/supabase/functions/passive-mining/index.ts @@ -1,7 +1,10 @@ import { Context, Hono } from "npm:hono@4.7.4"; import { createSupabaseAdmin } from "../_shared/supabase.ts"; import { getFolders } from "./boxes.ts"; -import { isReauthFailure } from "./credential-failure.ts"; +import { + isReauthFailure, + shouldNotifyStuckReauth, +} from "./credential-failure.ts"; import { type MiningSourceConfigV1, parseConfig, @@ -95,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; @@ -136,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,10 +178,7 @@ app.post("/", async (c: Context) => { const needsReauth = isReauthFailure(error); await recordRunFailure(miningSource.id, errorMessage, needsReauth); if (needsReauth) { - await notifyCredentialFailure( - miningSource.user_id, - miningSource.email, - ); + await notifyReauthOnce(miningSource); } } } @@ -174,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") @@ -193,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) { From 898fa9d8f8814ba6b3a7afceccdd5241bf63a5d1 Mon Sep 17 00:00:00 2001 From: Malek Salem Date: Fri, 23 Oct 2026 14:40:57 +0100 Subject: [PATCH 4/4] test(mining): drop redundant undefined arguments flagged by DeepSource --- .../passive-mining/credential-failure.test.ts | 14 ++++---------- .../functions/passive-mining/credential-failure.ts | 4 ++-- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/supabase/functions/passive-mining/credential-failure.test.ts b/supabase/functions/passive-mining/credential-failure.test.ts index b7798635e..88472d262 100644 --- a/supabase/functions/passive-mining/credential-failure.test.ts +++ b/supabase/functions/passive-mining/credential-failure.test.ts @@ -20,7 +20,7 @@ Deno.test("other statuses are not reauth failures", () => { Deno.test("a flagged source is notified once", () => { assertEquals( - shouldNotifyStuckReauth(SourceHealthState.NeedsReauth, undefined), + shouldNotifyStuckReauth(SourceHealthState.NeedsReauth), true, ); assertEquals( @@ -33,13 +33,7 @@ Deno.test("a flagged source is notified once", () => { }); Deno.test("only flagged sources use the stuck-notification path", () => { - assertEquals( - shouldNotifyStuckReauth(SourceHealthState.Active, undefined), - false, - ); - assertEquals( - shouldNotifyStuckReauth(SourceHealthState.Error, undefined), - false, - ); - assertEquals(shouldNotifyStuckReauth(undefined, undefined), false); + 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 647244f0e..7337235c3 100644 --- a/supabase/functions/passive-mining/credential-failure.ts +++ b/supabase/functions/passive-mining/credential-failure.ts @@ -18,8 +18,8 @@ export function isReauthFailure(error?: unknown): boolean { * to tell the user about one that was flagged before they could be notified. */ export function shouldNotifyStuckReauth( - state: unknown, - notifiedAt: unknown, + state?: unknown, + notifiedAt?: unknown, ): boolean { return state === SourceHealthState.NeedsReauth && !notifiedAt; }