withMonitor causes server to crash when callback throws an error #16749
Description
Is there an existing issue for this?
- I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
- I have reviewed the documentation https://docs.sentry.io/
- I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/node
SDK Version
9.32.0
Framework Version
No response
Link to Sentry event
No response
Reproduction Example/SDK Setup
Repro repo here: https://github.com/zachkirsch/sentry-with-monitor-repro
Summary: If the callback passed to withMonitor throws an error, Sentry crashes the server.
Steps to Reproduce
import * as Sentry from "@sentry/node";
import express from "express";
const app = express();
app.get("/crash", async (_req, res) => {
try {
await Sentry.withMonitor("my monitor", async () => {
throw new Error("Oops!");
});
res.sendStatus(200);
} catch (error) {
console.log("Error caught");
res.sendStatus(500);
}
});
app.listen(8000, () => {
console.log("Server is running on port 8000");
console.log("To crash server: curl http://localhost:8000/crash");
});
Expected Result
In the code above, when curling the /crash endpoint, a 500 status should be returned to the user and the server should stay alive listening for more requests.
Actual Result
In the code above, when curling the /crash endpoint, the entire server crashes:
I believe the fix should be to delete this line:
The Promise.resolve(maybePromiseResult).then( is effectively run "in the background" and doesn't need to throw. maybePromiseResult (the result of the user's callback) is returned from withMonitor, and it's already the user's responsibility to catch it if it rejects.
Workarounds?
The only workaround is to not use withMonitor and instead rely on imperative methods like Sentry.captureCheckIn. When using withMonitor, you can't wrap your callback in a try/catch because then withMonitor would mark all crons as successful.