fix(cron): auto-disable cron jobs whose target channel is unreachable#211
fix(cron): auto-disable cron jobs whose target channel is unreachable#211LIU9293 wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 566300571b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| try { | ||
| patchCronJob(job.id, { enabled: false }); |
There was a problem hiding this comment.
When a cron row points at a channel that has since been removed from the Ode config, this patchCronJob call re-enters updateCronJob(), which calls getChannelSnapshot() and throws before enabled is flipped. In that stale-config/unreachable-channel case the new auto-disable path only logs the failure, leaves the row enabled, and the scheduler continues firing and producing the same permanent channel_not_found/missing-access error on every tick; the same pattern is duplicated in the failure-notification path below.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 46fe172. Added disableCronJob(id) direct-SQL helper that flips enabled = 0 without re-validating channel config via getChannelSnapshot, so the stale-channel case (which is exactly the scenario this PR targets) now actually disables the row instead of silently swallowing the error. Both call sites switched. New test asserts patchCronJob throws while disableCronJob succeeds when the channel was removed from ode.json.
Addresses Codex P2 review on PR #211. The previous patch routed auto-disable through patchCronJob -> updateCronJob -> getChannelSnapshot, which throws "Channel not found in configured workspaces" when the cron row's destination channel has since been removed from ode.json. In that exact case (stale local config, bot kicked, workspace re-onboarded) the auto-disable was silently swallowed and the row stayed enabled — the scheduler would keep firing and capturing the same channel_not_found Sentry event every tick, defeating the point of the PR. Add a direct-SQL disableCronJob(id) helper that flips enabled=0 without re-validating channel config, and use it from both call sites (success-path send and failure-notification send). Tests: new disableCronJob suite covers - happy path (enabled true -> false, returns true) - idempotent re-disable (returns false) - missing row (returns false) - stale-channel case where patchCronJob throws but disableCronJob still succeeds (the regression Codex flagged) bun test: 418 pass, 1 skip, 0 fail.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46fe1725ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const code = (err as { code?: unknown }).code; | ||
| if (typeof code === "number" && PERMANENT_DISCORD_CODES.has(code)) { | ||
| return true; |
There was a problem hiding this comment.
For Discord cron sends, this numeric-code check usually never sees the DiscordAPIError: sendChannelMessage calls resolveTextChannel, which catches channels.fetch errors and then rethrows a generic Error("Discord channel ... is not text-based or inaccessible") (packages/ims/discord/client.ts:143-157). In the deleted/private-channel cases this change is meant to auto-disable, the error arriving here has no code, so the job stays enabled and keeps retrying instead of being disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in caf32bf. resolveTextChannel now captures err.code from every client.channels.fetch() attempt and forwards a representative permanent code (10003 / 50001 / 50013 / 50007 preferred) on the thrown wrapper Error, so isPermanentChannelError can classify it via the existing numeric-code shape. The wrapper message is unchanged, and a new test in permanent-error.test.ts asserts the wrapper-with-code path is treated as permanent.
… wrapper Addresses Codex P2 review on PR #211. resolveTextChannel() catches each client.channels.fetch() failure and then rethrows a generic Error("Discord channel ... is not text-based or inaccessible") with no `code` attached. That generic error reaches isPermanentChannelError(), which checks for numeric DiscordAPIErrorcode (10003 Unknown Channel / 50001 Missing Access / etc.) and falls through to false — so a Discord cron job whose bot has been removed from the target channel stays enabled and keeps firing every tick, exactly the noise pattern this PR exists to fix. Capture every Discord API code from each fetch attempt, pick a permanent one (10003 / 50001 / 50013 / 50007) when available, and attach it to the thrown wrapper as `code` (with the full list on `discordErrorCodes` for diagnostics). The wrapper's message is unchanged so callers that match on the string still work. Add a permanent-error.test.ts case asserting the wrapper Error with a forwarded code is classified as permanent. bun test: 419 pass, 1 skip, 0 fail.
When a cron job's send hits a permanent channel-access error (channel_not_found, not_in_channel, is_archived, token_revoked, etc.), the scheduler now disables the job and records the reason. Previously the same broken job would re-fire on every cron tick and capture an identical Sentry event for the lifetime of the daemon — see Sentry ODE-DEAMON-7 (5 events over ~5 weeks from one stuck job). - Add @/shared/delivery/permanent-error with cross-platform detection (Slack error strings, Discord numeric codes, Lark chat_not_found) and intentionally narrow classification: transient failures stay retryable. - Wire it into both the success-path send and the failure-notification send in runCronJob. The check on the notify path uses the *notify* error, not the original turn error, so a transient agent timeout doesn't disable a job when the channel is fine. - Add unit tests covering Slack/Discord/Lark error shapes and ensuring rate-limits / 5xx / ECONNRESET / message_not_found stay retryable.
Addresses Codex P2 review on PR #211. The previous patch routed auto-disable through patchCronJob -> updateCronJob -> getChannelSnapshot, which throws "Channel not found in configured workspaces" when the cron row's destination channel has since been removed from ode.json. In that exact case (stale local config, bot kicked, workspace re-onboarded) the auto-disable was silently swallowed and the row stayed enabled — the scheduler would keep firing and capturing the same channel_not_found Sentry event every tick, defeating the point of the PR. Add a direct-SQL disableCronJob(id) helper that flips enabled=0 without re-validating channel config, and use it from both call sites (success-path send and failure-notification send). Tests: new disableCronJob suite covers - happy path (enabled true -> false, returns true) - idempotent re-disable (returns false) - missing row (returns false) - stale-channel case where patchCronJob throws but disableCronJob still succeeds (the regression Codex flagged) bun test: 418 pass, 1 skip, 0 fail.
… wrapper Addresses Codex P2 review on PR #211. resolveTextChannel() catches each client.channels.fetch() failure and then rethrows a generic Error("Discord channel ... is not text-based or inaccessible") with no `code` attached. That generic error reaches isPermanentChannelError(), which checks for numeric DiscordAPIErrorcode (10003 Unknown Channel / 50001 Missing Access / etc.) and falls through to false — so a Discord cron job whose bot has been removed from the target channel stays enabled and keeps firing every tick, exactly the noise pattern this PR exists to fix. Capture every Discord API code from each fetch attempt, pick a permanent one (10003 / 50001 / 50013 / 50007) when available, and attach it to the thrown wrapper as `code` (with the full list on `discordErrorCodes` for diagnostics). The wrapper's message is unchanged so callers that match on the string still work. Add a permanent-error.test.ts case asserting the wrapper Error with a forwarded code is classified as permanent. bun test: 419 pass, 1 skip, 0 fail.
LIU9293
force-pushed
the
fix/cron-auto-disable-channel-not-found-04526a90
branch
from
caf32bf to
abc106f
Compare
|
Rebased onto latest Full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc106fe48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
When multiple Discord bots are configured, resolveTextChannel collects errors from every client; if the owning bot's channels.fetch() fails transiently but another workspace's bot returns Missing Access/Unknown Channel, this prioritizes the unrelated permanent code and the cron scheduler will auto-disable the job even though a retry with the owning bot could succeed. Cron top-level sends call sendDiscordChannelMessage(job.channelId, ...) without a processor/workspace id, so this mixed-attempt path is reachable in multi-workspace daemons.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e1a33ea. resolveTextChannel now tracks the pinned bot's error code separately from the multi-bot fan-out, with this rule for the wrapper's forwarded code:
- Pinned bot was attempted → forward its code (whatever it was). The pinned bot owns the channel, so its outcome is authoritative; unrelated bots' permanent codes for the same channel are noise.
- No pinned bot (e.g. cron top-level send without a processorId) → only forward a permanent code if every captured code is permanent. Mixed transient+permanent means at least one bot might succeed on retry, so the wrapper deliberately does NOT carry a permanent code.
New test ignores Discord resolveTextChannel wrapper without a forwarded code confirms that a wrapper with no code (or a transient code like 500) is not classified as permanent, so the cron scheduler will not auto-disable on a mixed-bot fan-out. Full suite green (435 pass, 1 skip).
| "missing_scope", | ||
| // Lark | ||
| "chat_not_found", | ||
| "chat not exist", |
There was a problem hiding this comment.
When the Lark send API returns a payload whose msg is just chat does not exist, the Lark client throws that message directly from larkApi, but this token does not match because it omits does. The new test only passes because it includes chat_not_found; deleted/stale Lark chat cron jobs that report the documented-style text therefore take the generic failure path and remain enabled instead of being auto-disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e1a33ea. Added "chat does not exist" as an explicit token in PERMANENT_MESSAGE_TOKENS — the shorter "chat not exist" does not match this string because of the "does" infix, so deleted/stale Lark chats whose larkApi payload surfaces as just "chat does not exist" now classify as permanent and the cron row auto-disables. New test matches Lark deleted-chat message text alone covers the bare-string case; full suite still green (435 pass, 1 skip).
…t failures Addresses two unaddressed Codex review comments on PR #211: 1. Lark deleted/stale chats: the Lark client re-throws the raw 'msg' field for some payloads, which surfaces as 'chat does not exist'. The shorter 'chat not exist' token does not match (the 'does' infix breaks the substring), so cron jobs targeting deleted Lark chats stayed enabled. Add 'chat does not exist' as an explicit token. 2. Discord multi-bot fetch races: when no processorId pins a bot, resolveTextChannel iterates every Discord client; mixing one bot's transient error with another (unrelated) bot's 'Missing Access' would forward the permanent code on the wrapper and auto-disable the cron job even though a retry could succeed. Track the pinned bot's code separately and only treat the multi-bot wrapper as permanent when either (a) the pinned bot returned a code, or (b) every captured code is permanent. Tests: - Added 'matches Lark deleted-chat message text alone' covering the bare 'chat does not exist' string. - Added 'ignores Discord resolveTextChannel wrapper without a forwarded code' covering the mixed-bot case (no code or transient code on the wrapper). - Full suite green: 435 pass, 1 skip, 0 fail.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1a33ea782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allPermanent = discordErrorCodes.every((c) => PERMANENT_PRIORITY.has(c)); | ||
| if (allPermanent) { | ||
| forwardedCode = discordErrorCodes.find((c) => PERMANENT_PRIORITY.has(c)); |
There was a problem hiding this comment.
This still auto-disables a Discord cron job when a code-less transient fetch failure is mixed with an unrelated bot's permanent error: allPermanent only checks discordErrorCodes, so an ECONNRESET/network error from the bot that could own the channel is ignored while another workspace bot's Missing Access/Unknown Channel code makes the wrapper look permanent. Cron top-level sends call this without a processor id, so in multi-workspace daemons this can disable a healthy job after a single transient fetch failure. Fresh evidence after the prior mixed-bot comment is that the current guard counts captured numeric codes rather than all failed attempts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1820940. The unpinned-bot path now tracks totalFailedAttempts (incremented in captureCode for every thrown error, regardless of whether it carries a numeric code) and only promotes to permanent when permanentCodes.length === totalFailedAttempts. A bare ECONNRESET (no code) from one bot mixed with 50001 from another now falls through to the non-permanent branch, so the cron row stays enabled and the next tick can retry. Full suite green (436 pass, 1 skip).
Address Codex review: in resolveTextChannel's unpinned-bot path, the allPermanent check only inspected captured numeric discordErrorCodes. A transient failure without a numeric code (ECONNRESET, network error) would be absent from that list, so an unrelated bot's permanent code (Missing Access / Unknown Channel) could promote the wrapper to permanent and auto-disable a healthy job on a single transient blip. Track totalFailedAttempts alongside the captured codes; only promote to permanent when permanentCodes.length === totalFailedAttempts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18209406f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const realThreadId = await sendResultToChannel(job, finalText); | ||
| let realThreadId: string | undefined; | ||
| try { | ||
| realThreadId = await sendResultToChannel(job, finalText); |
There was a problem hiding this comment.
When a Lark cron job's channel/workspace credentials have been removed, sendLarkChannelMessage returns undefined instead 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.
Addressed in 1fdf52b. sendResultToChannel now checks the Lark return value and throws Lark send returned no message id for channel <id> (chat_not_found or credentials missing) when sendLarkChannelMessage returns undefined (the missing-credentials path at packages/ims/lark/client.ts:383-384). The wrapper text embeds chat_not_found, so the existing isPermanentChannelError check in the runCronJob catch branch classifies it as permanent and disableCronJobForPermanentChannelError disables the job — same terminal behavior as an explicit chat_not_found API 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.
When a Lark cron job's channel/workspace credentials are removed from config, sendLarkChannelMessage returns undefined instead of throwing (packages/ims/lark/client.ts:383-384). Before this fix, runCronJob treated the missing message id as a successful delivery, called markCronJobCompleted, and the job stayed enabled and silently undelivered on every subsequent tick — the auto-disable helper introduced earlier in this PR was never reached. sendResultToChannel now throws an error whose message contains chat_not_found when the Lark sender returns undefined, so the existing isPermanentChannelError check in the runCronJob catch branch fires and disableCronJobForPermanentChannelError disables the job the same way it does for explicit chat_not_found responses. Added a unit test asserting the exact wrapper message classifies as permanent, so future edits to the wrapper text cannot silently regress the auto-disable path. Addresses PR #211 review comment 'Treat undefined sends as delivery failures'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fdf52b2fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (allAttemptsPermanent) { | ||
| forwardedCode = permanentCodes[0]; | ||
| } else { | ||
| forwardedCode = discordErrorCodes[0]; |
There was a problem hiding this comment.
In an unpinned multi-bot Discord send, if one fetch fails with a permanent code and another fetch fails with any non-permanent numeric DiscordAPIError, allAttemptsPermanent is false but this branch still forwards discordErrorCodes[0]; when the first captured code is 10003/50001/etc., isPermanentChannelError will auto-disable the cron job even though not every bot failure was permanent. Fresh evidence after the prior mixed-failure fix is that the current fallback still exposes the first captured numeric code to the classifier instead of suppressing it when the attempts are mixed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in a831e9d. The unpinned mixed-failure branch no longer forwards discordErrorCodes[0] as the wrapper's code when it happens to be permanent — instead it forwards the first non-permanent numeric code (still useful diagnostic signal without triggering isPermanentChannelError), and leaves code undefined when every captured numeric code is permanent but an untyped transient error is also in play (totalFailedAttempts > permanentCodes.length).
New regression test in permanent-error.test.ts — "does not classify wrapper as permanent when only discordErrorCodes carries permanent codes" — locks in the wrapper-shape guarantee: a wrapper carrying only discordErrorCodes: [10003, 50035] with no top-level code must not classify as permanent, since the classifier only inspects code.
bun test → 438 pass, 1 skip, 0 fail; bun run typecheck clean.
…ilures When resolveTextChannel encounters a mixed set of unpinned-bot failures (one permanent DiscordAPIError code, plus a non-permanent numeric code from an unrelated bot), the previous fallback still forwarded discordErrorCodes[0] as the wrapper's `code`. If the first captured code happened to be permanent (10003 / 50001 / 50013 / 50007), isPermanentChannelError promoted the wrapper to permanent and the cron job was auto-disabled — even though at least one retryable failure was in the mix and a retry against the other bot might have succeeded. The unpinned mixed branch now forwards only the first non-permanent numeric code (still a useful diagnostic signal on the wrapper) and leaves `code` undefined when every captured code is permanent but totalFailedAttempts exceeds permanentCodes.length (i.e. an untyped transient error is also in play). The full discordErrorCodes array is preserved on the wrapper for diagnostics. Adds a regression test in permanent-error.test.ts asserting that a wrapper carrying only `discordErrorCodes` (no top-level `code`) is not classified as permanent, even when the array contains permanent codes. Addresses PR #211 review 'Avoid forwarding a permanent code after a mixed Discord failure' (chatgpt-codex-connector, 2026-07-06).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a831e9d702
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allAttemptsPermanent = | ||
| permanentCodes.length === totalFailedAttempts && totalFailedAttempts > 0; | ||
| if (allAttemptsPermanent) { | ||
| forwardedCode = permanentCodes[0]; |
There was a problem hiding this comment.
In a multi-workspace daemon, this unpinned path only considers the Discord clients that are currently connected; if the cron job's own workspace client failed to log in or is temporarily restarting, every remaining bot can return permanent-looking 10003/50001 for that channel, causing isPermanentChannelError to auto-disable a job that would recover once its configured bot reconnects. Fresh evidence after the mixed-bot fixes is that this guard checks only failed attempts from available clients, while cron top-level sends still are not pinned to the job's stored workspace.
Useful? React with 👍 / 👎.
1 participant
What
When a cron job's send hits a permanent channel-access error (
channel_not_found,not_in_channel,is_archived,token_revoked, DiscordMissing Access/Unknown Channel, Larkchat_not_found, …), the scheduler now disables the job and records the reason inlast_error. The cron row stops firing until a human re-enables it.Why
Sentry ODE-DEAMON-7 — "IM slack send failed: channel_not_found" — captured 5 events over ~5 weeks, all from a single cron job whose bot had been removed from channel
C0ATGCJ0YK0. Without intervention the job would keep firing every cron tick forever, each failure capturing a fresh Sentry event with the same fingerprint. This converts that infinite drip into a one-shot auto-disable + log.This is a behaviour fix, not a Sentry-noise suppression. The Sentry event already correctly indicates "the bot may have been removed" (see existing
isBenignDeliveryFailuretest); the right product response is to stop firing the job, which is what this PR does.Design notes
packages/shared/delivery/permanent-error.tsalongside the existingisRateLimitError. It is intentionally narrow: transient failures (5xx, ECONNRESET, 429, socket hangups, Slackmessage_not_found) explicitly stay retryable.runCronJob:patchCronJob({ enabled: false })+markCronJobFailed+ agent_resultfailAgentResult) is wrapped in best-effort try/catch so a SQLite hiccup never shadows the original delivery error.Tests
permanent-error.test.tscover Slack message strings, Slack SDKdata.errorshape, Discord numericcodes (50001, 10003), Lark messages, and explicit negatives (rate limits, ECONNRESET, 5xx,message_not_found).bun test→ 414 pass, 1 skip, 0 fail.Out of scope
/inviteevent, which is a separate observability problem; for nowode cron enable <id>is the recovery path.ode task) — tasks are single-shot so the noise pattern is bounded; cron is the recurring case that needs this guard.