Skip to content

fix: robustness — DB outages, crashing events, shutdown and orphaned tickets - #32

Merged
Musiker15 merged 6 commits into
MSK-Scripts:mainfrom
Uncharted-Roleplay:fix/robustness
Oct 23, 2026
Merged

Musiker15 merged 6 commits into
MSK-Scripts:mainfrom
Uncharted-Roleplay:fix/robustness

Conversation

@shane615

@shane615 shane615 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

Robustness fixes found while reviewing the bot before deploying it for our community (Uncharted Roleplay). Each commit is one change:

  • A PostgreSQL restart crashed the bot. The pg pool had no error listener, so an idle client losing its connection emitted an unhandled error event. The pool now logs it (it reconnects on the next query) and has a 10 s connectionTimeoutMillis.
  • One failing event took the process down. discord.js does not await listeners, so a rejected execute() (e.g. a DB error in messageCreate) became an unhandled rejection, which ends the process on current Node versions. Event handlers are now awaited in a try/catch, index.js logs unhandled rejections, exits cleanly on an uncaught exception, and the client error event has a listener.
  • Graceful shutdown. On SIGTERM/SIGINT the bot waits (up to 7 s, below the supervisor's 10 s SIGKILL) for an in-flight close to finish, then destroys the client and closes the DB pool, so a restart cannot leave the channel closed but the DB row open.
  • Tickets stayed "open" when their channel was gone. Deleting a ticket with createTranscript: false, deleting the channel by hand, or deleting it while the bot was offline left the row open. It counted against maxTicketOpened forever (the user could never open a ticket again) and auto-close / the staff reminder skipped it silently. Now: captureFinalTranscript closes the row even without a transcript, a new channelDelete event closes it, the background loops close it when Discord answers Unknown Channel (10003, transient errors are ignored), and open tickets are reconciled once at startup.
  • closeTicket() no longer wipes an earlier transcript when called without a new one (COALESCE), e.g. for a ticket that was closed, reopened and then orphaned.
  • Database passwords are masked in malformed-DATABASE_URL errors and in the db:migrate output.

New locale key messages.orphanedTicketReason, translated in all languages.

Related Issue

None opened; these came out of a code review. Happy to open issues if you prefer that workflow.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that changes existing behavior)
  • 🌍 Translation / locale update
  • 📖 Documentation only
  • 🔧 Refactor / chore (no functional change)

How Was This Tested?

  • Node.js version: 24.20.0
  • npm test: all existing tests pass, plus new node:test suites (tests/orphaned-tickets.test.js, tests/url-redaction.test.js) that run against an in-memory SQLite database.
  • Live test (2026-10-23): on our test server (PostgreSQL 16): a Postgres restart, and the bot's connections killed 11 times in 60 s while tickets were opened (no restart, tickets created); open, close with channel deletion, and a ticket channel deleted by hand (row closed at once). All passed.

Checklist

  • My code follows the project's conventions (CommonJS, no TypeScript, tb_ custom-id prefix, client.logger, etc.)
  • No new production dependencies were added (or it was discussed in an issue first)
  • User-facing strings go through client.t(...) and all three locale files (main.json, en.json, de.json) are in sync (all eight locale files, in fact)
  • Any DB schema change uses an inline migration in initDatabase() (no schema change; only the closeTicket UPDATE changed)
  • The bot still works without MSK_API_KEY (premium features degrade gracefully)
  • The bot starts cleanly (npm start) and I verified my change on a test server (see above)
  • I updated the relevant docs (README.md / README_GER.md, config.example.jsonc, CLAUDE.md) where behavior, config keys, commands or the schema changed (no config keys or commands changed)
  • I added a CHANGELOG.md entry under [Unreleased]
  • My commit messages are in English and follow Conventional Commits (feat:, fix:, docs:, …)

Note on the related PRs

This is one of three PRs from the same review (robustness, permissions, transcripts). They are independent and each applies to main on its own, but they touch some of the same lines (ticketActions.js exports, ready.js, CHANGELOG.md). Following your order: this one first, then #34, then #33; I will rebase the other two after each merge.

https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH

@Musiker15

Copy link
Copy Markdown
Member

Thanks a lot for this, and for splitting the review into three focused PRs. That made it easy to go through.

I checked every point against the current main and all of them still apply there: the missing error listener on the pg pool, the un-awaited event handlers (only dashboard.js has an unhandledRejection handler, the bot process itself doesn't), no graceful shutdown, tickets staying open when createTranscript is off or the channel gets deleted by hand, closeTicket() wiping an earlier transcript, and the password in DATABASE_URL errors. The 7 s shutdown wait also fits nicely under the supervisor's 10 s SIGKILL.

Tests pass on your branch (179/179).

One small nit: redactUrl now sits between the JSDoc block of parseDatabaseUrl and the function itself, so that doc comment ends up attached to the wrong function. Could you move redactUrl above the comment?

The branch currently conflicts with main, mostly in CHANGELOG.md because there is a new [Unreleased] block for a dependency update. Once you've tested it on your server and rebased, feel free to mark it ready and I'll merge it. I'd like to merge this one first, then #34, then #33.

- PostgreSQL pool: handle the idle-client 'error' event (it crashed the
  process on a Postgres restart) and set a 10 s connectionTimeoutMillis.
- Event handlers are awaited inside a try/catch, so a failing event such as
  a DB error in messageCreate no longer becomes an unhandled rejection.
- index.js: log unhandled rejections instead of exiting, exit cleanly on an
  uncaught exception, and on SIGTERM/SIGINT let an in-flight ticket close
  finish (up to 7 s) before destroying the client and closing the DB pool.
- Listen for the client 'error' event.

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
A ticket stayed "open" in the DB when its channel was deleted with
transcripts disabled, deleted by hand in Discord, or deleted while the bot
was offline. Those rows counted against maxTicketOpened forever (locking the
user out) and were skipped silently by auto-close and the staff reminder.

- captureFinalTranscript closes the row even when createTranscript is off.
- New channelDelete event closes the ticket of a deleted channel.
- Auto-close and staff-reminder loops close a ticket when Discord answers
  Unknown Channel (10003); transient fetch errors leave it alone.
- On startup, open tickets are reconciled against existing channels.
- New locale key messages.orphanedTicketReason (all languages).

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
… one

closeTicket() overwrote the transcript column with NULL whenever no new
transcript was passed. A ticket closed (transcript stored), reopened and
then orphaned — channel deleted by hand or while the bot was offline — lost
its original transcript, including in the dashboard. The column is now only
replaced when a new transcript is given.

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
- Database URLs are logged with the password masked: malformed-URL errors
  and the migrate script printed them verbatim. Masking runs up to the last
  "@", so an unencoded "@" in the password does not leak its tail.
- uncaughtException sets the exit code and exits after 200 ms, so the log
  line can drain from a piped stdout (dashboard supervisor).

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
Review feedback: the JSDoc block belonged to parseDatabaseUrl but ended up
attached to redactUrl.

Claude-Session: https://claude.ai/code/session_01HZPZxD3edzJNFFsiWsHYY1
@shane615

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Done:

  • Moved redactUrl above the parseDatabaseUrl doc comment.
  • Rebased onto main (v2.19.3), CHANGELOG.md conflict resolved.

Tested live today on our test server (PostgreSQL 16, bot without Administrator). Postgres restarted once on its own, and we also killed the bot's database connections 11 times in 60 seconds while tickets were being opened. The bot logged each reconnect, never restarted, and both tickets were created. A ticket channel deleted by hand was closed in the database straight away. Marking it ready.

The live test also turned up two bugs that exist on main as well. I'll send them as two small separate PRs so they don't hold these three up:

  • A bot without Administrator locks itself out of every new ticket channel: the @everyone deny applies to the bot too, so the opening message fails with Missing Access.
  • On PostgreSQL, BIGINT timestamps come back as strings, so every transcript fails with RangeError: Invalid time value.

@shane615
shane615 marked this pull request as ready for review October 23, 2026 20:09
@Musiker15
Musiker15 merged commit 1dcbbc5 into MSK-Scripts:main Oct 23, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants