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 2 commits into
Conversation
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.
![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 👍 / 👎.
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.