-
Notifications
You must be signed in to change notification settings - Fork 6
fix(cron): auto-disable cron jobs whose target channel is unreachable #211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
b66828f
4decf95
abc106f
e1a33ea
1820940
1fdf52b
a831e9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,20 @@ async function maybeHandleLauncherCommand(params: { | |
| } | ||
| async function resolveTextChannel(channelId: string, processorId?: string) { | ||
| const attempts: string[] = []; | ||
| // Collect Discord API error codes across every fetch attempt so the | ||
| // caller can detect permanent-access failures (Unknown Channel / Missing | ||
| // Access / etc.) even though we re-throw a generic wrapper Error below. | ||
| // discord.js surfaces these on `err.code` as numbers; we forward them on | ||
| // the wrapper as `discordErrorCodes` and also expose the first as `code` | ||
| // so `isPermanentChannelError` can pick it up via the `code` shape. | ||
| const discordErrorCodes: number[] = []; | ||
| const captureCode = (error: unknown) => { | ||
| if (typeof error === "object" && error !== null) { | ||
| const code = (error as { code?: unknown }).code; | ||
| if (typeof code === "number") discordErrorCodes.push(code); | ||
| } | ||
| }; | ||
|
|
||
| if (processorId) { | ||
| const pinnedClient = discordClientByProcessorId.get(processorId); | ||
| if (pinnedClient) { | ||
|
|
@@ -127,6 +141,7 @@ async function resolveTextChannel(channelId: string, processorId?: string) { | |
| } | ||
| attempts.push(`bot=${pinnedClient.user?.id || "unknown"}: channel_not_text_or_missing`); | ||
| } catch (error) { | ||
| captureCode(error); | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| attempts.push(`bot=${pinnedClient.user?.id || "unknown"}: ${errorMessage}`); | ||
| } | ||
|
|
@@ -142,6 +157,7 @@ async function resolveTextChannel(channelId: string, processorId?: string) { | |
| } | ||
| attempts.push(`bot=${client.user?.id || "unknown"}: channel_not_text_or_missing`); | ||
| } catch (error) { | ||
| captureCode(error); | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| attempts.push(`bot=${client.user?.id || "unknown"}: ${errorMessage}`); | ||
| } | ||
|
|
@@ -155,7 +171,20 @@ async function resolveTextChannel(channelId: string, processorId?: string) { | |
| }); | ||
| } | ||
|
|
||
| throw new Error(`Discord channel ${channelId} is not text-based or inaccessible`); | ||
| const wrapper = new Error(`Discord channel ${channelId} is not text-based or inaccessible`); | ||
| if (discordErrorCodes.length > 0) { | ||
| // Prefer the most informative code: prioritise "permanent" classes | ||
| // (10003 Unknown Channel, 50001 Missing Access, 50013 Missing Perms, | ||
| // 50007 Cannot DM) so isPermanentChannelError can disable the cron row | ||
| // even if some bots failed transiently. | ||
| const PERMANENT_PRIORITY = new Set([10003, 50001, 50013, 50007]); | ||
| const permanent = discordErrorCodes.find((c) => PERMANENT_PRIORITY.has(c)); | ||
| (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).code = | ||
| permanent ?? discordErrorCodes[0]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When multiple Discord bots are configured, Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in e1a33ea.
New test |
||
| (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).discordErrorCodes = | ||
| [...discordErrorCodes]; | ||
| } | ||
| throw wrapper; | ||
| } | ||
|
|
||
| async function buildDiscordContext( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { isPermanentChannelError } from "./permanent-error"; | ||
|
|
||
| describe("isPermanentChannelError", () => { | ||
| test("matches Slack channel_not_found in message", () => { | ||
| expect( | ||
| isPermanentChannelError( | ||
| new Error("An API error occurred: channel_not_found"), | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| test("matches Slack SDK error shape with data.error", () => { | ||
| const err = Object.assign(new Error("slack_webapi_platform_error"), { | ||
| data: { error: "channel_not_found" }, | ||
| }); | ||
| expect(isPermanentChannelError(err)).toBe(true); | ||
| }); | ||
|
|
||
| test("matches Slack not_in_channel / is_archived / account_inactive", () => { | ||
| expect(isPermanentChannelError(new Error("not_in_channel"))).toBe(true); | ||
| expect(isPermanentChannelError(new Error("is_archived"))).toBe(true); | ||
| expect(isPermanentChannelError(new Error("channel_is_archived"))).toBe(true); | ||
| expect(isPermanentChannelError(new Error("account_inactive"))).toBe(true); | ||
| }); | ||
|
|
||
| test("matches Slack auth-revoked errors", () => { | ||
| expect(isPermanentChannelError(new Error("token_revoked"))).toBe(true); | ||
| expect(isPermanentChannelError(new Error("invalid_auth"))).toBe(true); | ||
| expect(isPermanentChannelError(new Error("missing_scope"))).toBe(true); | ||
| }); | ||
|
|
||
| test("matches Discord-style numeric codes (Missing Access, Unknown Channel)", () => { | ||
| expect( | ||
| isPermanentChannelError(Object.assign(new Error("Missing Access"), { code: 50001 })), | ||
| ).toBe(true); | ||
| expect( | ||
| isPermanentChannelError(Object.assign(new Error("Unknown Channel"), { code: 10003 })), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| test("matches the Discord resolveTextChannel wrapper when it carries a DiscordAPIError code", () => { | ||
| // resolveTextChannel() in packages/ims/discord/client.ts wraps the | ||
| // underlying DiscordAPIError in a generic Error("... is not text-based | ||
| // or inaccessible"). The wrapper forwards the original numeric `code` | ||
| // so this helper can still classify it as permanent. | ||
| const wrapper = Object.assign( | ||
| new Error("Discord channel 123 is not text-based or inaccessible"), | ||
| { code: 10003 }, | ||
| ); | ||
| expect(isPermanentChannelError(wrapper)).toBe(true); | ||
| }); | ||
|
|
||
| test("matches Lark chat_not_found", () => { | ||
| expect(isPermanentChannelError(new Error("chat_not_found: chat does not exist"))).toBe(true); | ||
| }); | ||
|
|
||
| test("ignores transient / retryable failures", () => { | ||
| expect(isPermanentChannelError(new Error("status 429"))).toBe(false); | ||
| expect(isPermanentChannelError(new Error("ECONNRESET"))).toBe(false); | ||
| expect(isPermanentChannelError(new Error("status 500"))).toBe(false); | ||
| expect(isPermanentChannelError(new Error("socket hang up"))).toBe(false); | ||
| expect(isPermanentChannelError(new Error("rate_limited"))).toBe(false); | ||
| }); | ||
|
|
||
| test("ignores nullish / empty inputs", () => { | ||
| expect(isPermanentChannelError(null)).toBe(false); | ||
| expect(isPermanentChannelError(undefined)).toBe(false); | ||
| expect(isPermanentChannelError("")).toBe(false); | ||
| expect(isPermanentChannelError({})).toBe(false); | ||
| }); | ||
|
|
||
| test("ignores message_not_found (that's a delete/update race, not permanent)", () => { | ||
| // `message_not_found` is already handled as a benign update/delete race | ||
| // in @/core/observability/sentry. It is NOT a channel-access problem and | ||
| // must remain retryable from this helper's perspective. | ||
| expect(isPermanentChannelError(new Error("message_not_found"))).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a Lark cron job's channel/workspace credentials have been removed,
sendLarkChannelMessagereturnsundefinedinstead of throwing (packages/ims/lark/client.ts:383-384), so this await falls through the new permanent-error handling and the run is later marked completed while the job remains enabled and undelivered. Fresh evidence after the stale-channel fix is that the direct-SQL disable helper is never reached for this no-credentials Lark path because no error is raised.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 1fdf52b.
sendResultToChannelnow checks the Lark return value and throwsLark send returned no message id for channel <id> (chat_not_found or credentials missing)whensendLarkChannelMessagereturnsundefined(the missing-credentials path atpackages/ims/lark/client.ts:383-384). The wrapper text embedschat_not_found, so the existingisPermanentChannelErrorcheck in therunCronJobcatch branch classifies it as permanent anddisableCronJobForPermanentChannelErrordisables the job — same terminal behavior as an explicitchat_not_foundAPI response, no more silent completed-but-undelivered runs.Also added a test in
permanent-error.test.ts(matches the synthetic 'Lark send returned no message id' wrapper) that pins the exact wrapper string to the classifier, so a future rewording of the wrapper cannot silently regress the auto-disable path. Full suite: 437 pass, 1 skip, 0 fail.