Fix crashes when a ticket channel has no database entry - #108
Open
mods-hd wants to merge 1 commit into
Open
Conversation
Closing a channel with no ticket row wrote closeUserID before it read anything back, so the row got recreated as a stub holding only that field. getUser then fetched user "null" and the 400 came back as a null user, which crashed on ticketUserID.id. The stub also made ticketsDB.has() return true afterwards, so /delete sailed past its "not in a ticket" guard and died on ticketCategories[undefined].support_role_ids instead. Now closeTicket reads the creator first and bails with a message if it is missing, checkSupportRole returns false for an unknown button id, and the command error handler replies instead of editing when the interaction was never deferred.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I ran into this on a bot whose
tickets.sqlitehad lost rows for channels that still existed in Discord. Pressing the close button in one of those channels threw, and every attempt afterwards made things a little worse.The reason is the order of operations at the top of
closeTicket. It writescloseUserIDbefore it reads anything, so on a channel with no row quick.db happily creates one holding just that field. The very next line readsuserIDout of that same channel, getsnull, and hands it togetUser, which fires a request at/users/null:getUsercatches that and returnsnull, so a few lines later the embed field blows up onticketUserID.id:The stub row it left behind is the annoying part, because
ticketsDB.has()now returns true for that channel./deletechecks exactly that before anything else, so it walks straight past its "you are not in a ticket channel" guard and intocheckSupportRole, where the missingbuttonfield turns intoticketCategories[undefined]:And since
/deletethrows before itsdeferReply, the catch block ininteractionCreate.jstries toeditReplya reply that was never sent, so the user sees nothing at all and the log gets a second, misleading error:Three changes here.
closeTicketnow reads the ticket creator first and, if there is no creator or the fetch fails, tells the staff member the data is missing and returns without writing anything — so the stub row never gets created in the first place.checkSupportRolereturnsfalsewhen the button id is not in the configured categories instead of throwing. And the command error handler picksreplyoreditReplybased oninteraction.deferred || interaction.replied, so a command that fails before deferring still gets its error message through.On how this happens in the first place: mine came from a partial data restore, where
main.sqlitesurvived andtickets.sqlitedid not. A ticket deleted from the database while its channel stuck around would do it too. Either way the bot should say so rather than throw, and it should not corrupt the row on the way out.Two calls I would rather you made than me. The message in the early return is a bare string instead of a
config.errors.*lookup like everything else in the codebase — I left it inline to keep the diff to three files, but say the word and I will add the key toconfig.yml.exampleandlocale.yml.example. And the guard treats a failedgetUserthe same as missing data, so a transient API hiccup or a deleted account would now block the close rather than throw. That is no worse than the current behaviour, but if you would rather only the missing-row case be caught, that is a small change.On testing: this is running on my bot and it starts and operates normally, so nothing here regresses the usual paths. I have not been able to exercise the new guard itself, though, because I cleared the bad rows and deleted the orphaned channel before restarting, so there is no longer a channel in that state to click Close on. The reasoning for the early return is that all four callers of
closeTicket(commands/Tickets/close.js, plus three ininteractionCreate.js)deferReplyfirst, which is what makes theeditReplythere safe — worth a second pair of eyes.