-
Notifications
You must be signed in to change notification settings - Fork 3
fix: harden embed failure handling #97
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
Changes from 11 commits
5d39822
57d715c
f74e983
95a82ca
58b1237
79089f1
6aa7c92
f5ecdab
c35a65d
12cd0fc
04fba22
4a13608
cb93137
fd41c01
978c2b6
a4d0b16
63aa6ca
c818d0b
5e17a14
83eadd3
9d2eac5
eeee372
bc12621
106b6df
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 |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { EmbedlyErrors, EmbedlyLogs, formatLog, getErrorContext } from "@embedly/logging"; | ||
| import { Events, Listener } from "@sapphire/framework"; | ||
| import type { Message, PartialMessage } from "discord.js"; | ||
| import { DiscordAPIError, RESTJSONErrorCodes, type Message, type PartialMessage } from "discord.js"; | ||
|
|
||
| export class MessageDeleteListener extends Listener<typeof Events.MessageDelete> { | ||
| public constructor(context: Listener.LoaderContext, options: Listener.Options) { | ||
|
|
@@ -11,17 +11,51 @@ export class MessageDeleteListener extends Listener<typeof Events.MessageDelete> | |
| } | ||
|
|
||
| public async run(message: Message | PartialMessage) { | ||
| const botMessageIds = await this.container.messageCache.deleteSourceMessage(message.id); | ||
| const requestId = `message:${message.id}`; | ||
| let botMessageIds: string[]; | ||
| try { | ||
| botMessageIds = await this.container.messageCache.getBotMessageIds(message.id); | ||
| } catch (error) { | ||
| this.container.logger.warn( | ||
| formatLog("warn", EmbedlyErrors.MessageCacheFailed, { | ||
| request_id: requestId, | ||
| message_id: message.id, | ||
| ...getErrorContext(error), | ||
| }), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (botMessageIds.length === 0) return; | ||
|
|
||
| let deletedCount = 0; | ||
| for (const botMessageId of botMessageIds) { | ||
| try { | ||
| const botMessage = await message.channel.messages.fetch(botMessageId); | ||
| await botMessage.delete(); | ||
| deletedCount++; | ||
| } catch (error) { | ||
| if (error instanceof DiscordAPIError && error.code === RESTJSONErrorCodes.UnknownMessage) { | ||
| deletedCount++; | ||
| } else { | ||
| this.container.logger.warn( | ||
| formatLog("warn", EmbedlyErrors.DeleteFailed, { | ||
| request_id: requestId, | ||
| message_id: message.id, | ||
| bot_message_id: botMessageId, | ||
| ...getErrorContext(error), | ||
| }), | ||
| ); | ||
| continue; | ||
|
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 Discord returns a transient network, rate-limit, or server error after the source message has been deleted, this branch retains the generated-message mapping but schedules no retry. The mapping eventually expires, leaving the generated embed in Discord with no way to locate and delete it. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/bot/src/listeners/messageDelete.ts
Line: 49
Comment:
**Retained deletions are never retried**
When Discord returns a transient network, rate-limit, or server error after the source message has been deleted, this branch retains the generated-message mapping but schedules no retry. The mapping eventually expires, leaving the generated embed in Discord with no way to locate and delete it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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. Intentional best-effort behavior. This listener makes one Discord delete attempt, logs failures, and retains the mapping until TTL. We do not want a retry queue or repeated Discord API calls for a failed delete; Discord availability is outside this bot’s cleanup guarantee. Leaving this as-is. |
||
| } | ||
| } | ||
|
|
||
| try { | ||
| await this.container.messageCache.removeBotMessage(botMessageId); | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| } catch (error) { | ||
| this.container.logger.warn( | ||
| formatLog("warn", EmbedlyErrors.DeleteFailed, { | ||
| request_id: `message:${message.id}`, | ||
| formatLog("warn", EmbedlyErrors.MessageCacheFailed, { | ||
| request_id: requestId, | ||
| message_id: message.id, | ||
| bot_message_id: botMessageId, | ||
| ...getErrorContext(error), | ||
|
|
@@ -30,12 +64,19 @@ export class MessageDeleteListener extends Listener<typeof Events.MessageDelete> | |
| } | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| this.container.logger.info( | ||
| formatLog("info", EmbedlyLogs.AutoDeleteSucceeded, { | ||
| request_id: `message:${message.id}`, | ||
| message_id: message.id, | ||
| bot_message_count: botMessageIds.length, | ||
| }), | ||
| ); | ||
| const failedCount = botMessageIds.length - deletedCount; | ||
| const context = { | ||
| request_id: requestId, | ||
| message_id: message.id, | ||
| bot_message_count: botMessageIds.length, | ||
| deleted_count: deletedCount, | ||
| failed_count: failedCount, | ||
| }; | ||
| if (failedCount > 0) { | ||
| this.container.logger.warn(formatLog("warn", EmbedlyErrors.DeleteFailed, context)); | ||
| return; | ||
| } | ||
|
|
||
| this.container.logger.info(formatLog("info", EmbedlyLogs.AutoDeleteSucceeded, context)); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.