Skip to content

Conversation

@kurama
Copy link
Contributor

@kurama kurama commented Nov 24, 2025

❗ Important Announcements

Click here for more details:

⚠️ Please Note: We do not accept all types of pull requests, and we want to ensure we don’t waste your time. Before submitting, make sure you have read our pull request guidelines: Pull Request Rules

🚫 Please Avoid Unnecessary Pinging of Maintainers

We kindly ask you to refrain from pinging maintainers unless absolutely necessary. Pings are for critical/urgent pull requests that require immediate attention.

📋 Overview

What problem does this pull request address?
This PR fixes an error in the deleteMonitor socket handler that causes the application to crash with TypeError: callback is not a function. This occurs when the callback parameter is undefined or not properly passed in certain deletion scenarios, particularly when deleting monitors.

Errir trace (on master branch):

2025-11-24T12:14:55+01:00 [DB] INFO: Delete Monitor completed in : 24 ms
Trace: TypeError: callback is not a function
    at Socket.<anonymous> (/home/dgrasset/dev/uptime-kuma/server/server.js:1083:17)
    at process.unexpectedErrorHandler (/home/dgrasset/dev/uptime-kuma/server/server.js:1893:13)
    at process.emit (node:events:518:28)
    at emitUnhandledRejection (node:internal/process/promises:252:13)
    at throwUnhandledRejectionsMode (node:internal/process/promises:388:19)
    at processPromiseRejections (node:internal/process/promises:475:17)
    at process.processTicksAndRejections (node:internal/process/task_queues:106:32)
If you keep encountering errors, please report to https://github.com/louislam/uptime-kuma/issues

What features or functionality does this pull request introduce or enhance?
This PR adds defensive callback validation in the deleteMonitor handler by checking if the callback exists before invoking it.

Btw, I'm skeptical that there's no issue created for this.

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify):
    • Provide additional details here.

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any) I used an LLM for the contributions
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

📷 Screenshots or Visual Changes

  • UI Modifications: Highlight any changes made to the user interface.
  • Before & After: Include screenshots or comparisons (if applicable).
Event Before After
UP Before After
DOWN Before After
Certificate-expiry Before After
Testing Before After

Copilot AI review requested due to automatic review settings November 24, 2025 11:33
Copilot finished reviewing on behalf of kurama November 24, 2025 11:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a crash in the deleteMonitor socket handler that occurs when the callback parameter is undefined. The error happens because the handler's backward compatibility logic for an optional middle parameter (deleteChildren) doesn't guarantee that a callback will always be defined. The fix adds defensive checks before invoking the callback.

Key changes:

  • Added if (callback) validation before calling callback in success path
  • Added if (callback) validation before calling callback in error path

Comment on lines +1129 to +1135
if (callback) {
callback({
ok: true,
msg: "successDeleted",
msgi18n: true,
});
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the callback validation prevents the crash, this pattern is inconsistent with all other socket handlers in this file (e.g., pauseMonitor, resumeMonitor, deleteTag, deleteNotification) which always call callbacks without checking. The root issue is that the backward compatibility logic (lines 1059-1062) allows callback to be undefined when only monitorID is provided. Consider whether callbacks should be required for this handler (consistent with other handlers) or if the backward compatibility should explicitly handle the no-callback case with a default no-op function, e.g., callback = callback || (() => {}); after the backward compatibility check.

Copilot uses AI. Check for mistakes.
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.

1 participant