From 513d63f4143b1004eba5e759fab0d26bf17ed476 Mon Sep 17 00:00:00 2001 From: Hany Date: Thu, 22 May 2025 17:16:33 +0300 Subject: [PATCH] fix: prevent reporting expected module restart errors to Sentry --- lib/zinnia.js | 41 +++++++++--- package-lock.json | 159 ++++++++++++++++++++++++++++++++++++++++++++ package.json | 1 + test/zinnia.test.js | 25 +++++++ 4 files changed, 215 insertions(+), 11 deletions(-) create mode 100644 test/zinnia.test.js diff --git a/lib/zinnia.js b/lib/zinnia.js index 07acb279..7ec68dff 100644 --- a/lib/zinnia.js +++ b/lib/zinnia.js @@ -1,5 +1,5 @@ import { execa } from 'execa' -import * as Sentry from '@sentry/node' +import { withScope, captureException } from '@sentry/node' //imported here to support fingerprint grouping in Sentry import { installRuntime, getRuntimeExecutable } from './runtime.js' import { updateSourceFiles } from './subnets.js' import os from 'node:os' @@ -53,18 +53,37 @@ export const install = () => }) let lastErrorReportedAt = 0 -const maybeReportErrorToSentry = (/** @type {unknown} */ err) => { +/** @param {unknown} err */ +function isErrorWithSentryFlag(err) { + return typeof err === 'object' && err !== null && 'reportToSentry' in err +} + +/** + * Reports errors to Sentry, unless reportToSentry is explicitly false. Uses + * dependency injection to allow clean unit testing. + * + * @param {unknown} err + * @param {(error: unknown, hint?: object) => void} captureFn - Optional Sentry + * capture function (default: captureException) + */ +export function maybeReportErrorToSentry(err, captureFn = captureException) { + if ( + isErrorWithSentryFlag(err) && + /** @type {{ reportToSentry?: boolean }} */ (err).reportToSentry === false + ) { + return + } + const now = Date.now() if (now - lastErrorReportedAt < 4 /* HOURS */ * 3600_000) return lastErrorReportedAt = now - /** @type {Parameters[1]} */ + /** @type {Parameters[1]} */ const hint = { extra: {} } - if (typeof err === 'object') { - if ('reportToSentry' in err && err.reportToSentry === false) { - return - } + if (typeof err === 'object' && err !== null) { + // Mark error to prevent future duplicate reporting Object.assign(err, { reportToSentry: false }) + if ('details' in err && typeof err.details === 'string') { // Quoting from https://develop.sentry.dev/sdk/data-handling/ // > Messages are limited to 8192 characters. @@ -81,7 +100,7 @@ const maybeReportErrorToSentry = (/** @type {unknown} */ err) => { console.error( 'Reporting the problem to Sentry for inspection by the Checker team.', ) - Sentry.captureException(err, hint) + captureFn(err, hint) } const matchesSubnetFilter = (subnet) => @@ -232,7 +251,7 @@ const catchChildProcessExit = async ({ } else { // Apply a custom rule to force Sentry to group all issues with the same subnet & exit code // See https://docs.sentry.io/platforms/node/usage/sdk-fingerprinting/#basic-example - Sentry.withScope((scope) => { + withScope((scope) => { scope.setFingerprint([message]) maybeReportErrorToSentry(subnetErr) }) @@ -319,7 +338,7 @@ export async function run({ }) const err = new Error('Module inactive for 5 minutes') - Object.assign(err, { module }) + Object.assign(err, { module, reportToSentry: false }) maybeReportErrorToSentry(err) controller.abort() @@ -340,7 +359,7 @@ export async function run({ text: data, }).catch((err) => { console.error(err) - Sentry.captureException(err) + maybeReportErrorToSentry(err) }) }) childProcess.stderr.setEncoding('utf-8') diff --git a/package-lock.json b/package-lock.json index 28f00e04..846f2549 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,6 +42,7 @@ "prettier-plugin-jsdoc": "^1.3.2", "prettier-plugin-multiline-arrays": "^4.0.3", "prettier-plugin-packagejson": "^2.5.10", + "sinon": "^20.0.0", "stream-match": "^1.2.1", "typescript": "^5.0.4" }, @@ -2141,6 +2142,48 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/@sinonjs/commons": { + "version": "3.0.1", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-3.0.1.tgz", + "integrity": "sha512-K3mCHKQ9sVh8o1C9cxkwxaOmXoAMlDxC1mYyHrjqOWEcBjYr76t96zL2zlj5dUGZ3HSw240X1qgH3Mjf1yJWpQ==", + "dev": true, + "license": "BSD-3-Clause", + "dependencies": { + "type-detect": "4.0.8" + } + }, + "node_modules/@sinonjs/fake-timers": { + "version": "13.0.5", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-13.0.5.tgz", + "integrity": "sha512-36/hTbH2uaWuGVERyC6da9YwGWnzUZXuPro/F2LfsdOsLnCojz/iSH8MxUt/FD2S5XBSVPhmArFUXcpCQ2Hkiw==", + "dev": true, + "license": "BSD-3-Clause", + "dependencies": { + "@sinonjs/commons": "^3.0.1" + } + }, + "node_modules/@sinonjs/samsam": { + "version": "8.0.2", + "resolved": "https://registry.npmjs.org/@sinonjs/samsam/-/samsam-8.0.2.tgz", + "integrity": "sha512-v46t/fwnhejRSFTGqbpn9u+LQ9xJDse10gNnPgAcxgdoCDMXj/G2asWAC/8Qs+BAZDicX+MNZouXT1A7c83kVw==", + "dev": true, + "license": "BSD-3-Clause", + "dependencies": { + "@sinonjs/commons": "^3.0.1", + "lodash.get": "^4.4.2", + "type-detect": "^4.1.0" + } + }, + "node_modules/@sinonjs/samsam/node_modules/type-detect": { + "version": "4.1.0", + "resolved": "https://registry.npmjs.org/type-detect/-/type-detect-4.1.0.tgz", + "integrity": "sha512-Acylog8/luQ8L7il+geoSxhEkazvkslg7PSNKOX59mbB9cOveP5aq9h74Y7YU8yDpJwetzQQrfIwtf4Wp4LKcw==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=4" + } + }, "node_modules/@sovpro/delimited-stream": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/@sovpro/delimited-stream/-/delimited-stream-1.1.0.tgz", @@ -7752,6 +7795,14 @@ "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", "dev": true }, + "node_modules/lodash.get": { + "version": "4.4.2", + "resolved": "https://registry.npmjs.org/lodash.get/-/lodash.get-4.4.2.tgz", + "integrity": "sha512-z+Uw/vLuy6gQe8cfaFWD7p0wVv8fJl3mbzXh33RS+0oW2wvUqiRXiQ69gLWSLpgB5/6sU+r6BlQR0MBILadqTQ==", + "deprecated": "This package is deprecated. Use the optional chaining (?.) operator instead.", + "dev": true, + "license": "MIT" + }, "node_modules/lodash.merge": { "version": "4.6.2", "resolved": "https://registry.npmjs.org/lodash.merge/-/lodash.merge-4.6.2.tgz", @@ -10653,6 +10704,34 @@ "integrity": "sha512-wnD2ZE+l+SPC/uoS0vXeE9L1+0wuaMqKlfz9AMUo38JsyLSBWSFcHR1Rri62LZc12vLr1gb3jl7iwQhgwpAbGQ==", "dev": true }, + "node_modules/sinon": { + "version": "20.0.0", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-20.0.0.tgz", + "integrity": "sha512-+FXOAbdnj94AQIxH0w1v8gzNxkawVvNqE3jUzRLptR71Oykeu2RrQXXl/VQjKay+Qnh73fDt/oDfMo6xMeDQbQ==", + "dev": true, + "license": "BSD-3-Clause", + "dependencies": { + "@sinonjs/commons": "^3.0.1", + "@sinonjs/fake-timers": "^13.0.5", + "@sinonjs/samsam": "^8.0.1", + "diff": "^7.0.0", + "supports-color": "^7.2.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/sinon" + } + }, + "node_modules/sinon/node_modules/diff": { + "version": "7.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-7.0.0.tgz", + "integrity": "sha512-PJWHUb1RFevKCwaFA9RlG5tCd+FO5iRh9A8HEtkmBH2Li03iJriB6m6JIN4rGz3K3JLawI7/veA1xzRKP6ISBw==", + "dev": true, + "license": "BSD-3-Clause", + "engines": { + "node": ">=0.3.1" + } + }, "node_modules/slash": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/slash/-/slash-5.1.0.tgz", @@ -11228,6 +11307,16 @@ "node": ">= 0.8.0" } }, + "node_modules/type-detect": { + "version": "4.0.8", + "resolved": "https://registry.npmjs.org/type-detect/-/type-detect-4.0.8.tgz", + "integrity": "sha512-0fr/mIH1dlO+x7TlcMy+bIDqKPsw/70tVyeHW787goQjhmqaZe10uwLujubK9q9Lg6Fiho1KUKDYz0Z7k7g5/g==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=4" + } + }, "node_modules/type-fest": { "version": "4.39.1", "resolved": "https://registry.npmjs.org/type-fest/-/type-fest-4.39.1.tgz", @@ -13498,6 +13587,43 @@ "resolved": "https://registry.npmjs.org/@sindresorhus/merge-streams/-/merge-streams-4.0.0.tgz", "integrity": "sha512-tlqY9xq5ukxTUZBmoOp+m61cqwQD5pHJtFY3Mn8CA8ps6yghLH/Hw8UPdqg4OLmFW3IFlcXnQNmo/dh8HzXYIQ==" }, + "@sinonjs/commons": { + "version": "3.0.1", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-3.0.1.tgz", + "integrity": "sha512-K3mCHKQ9sVh8o1C9cxkwxaOmXoAMlDxC1mYyHrjqOWEcBjYr76t96zL2zlj5dUGZ3HSw240X1qgH3Mjf1yJWpQ==", + "dev": true, + "requires": { + "type-detect": "4.0.8" + } + }, + "@sinonjs/fake-timers": { + "version": "13.0.5", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-13.0.5.tgz", + "integrity": "sha512-36/hTbH2uaWuGVERyC6da9YwGWnzUZXuPro/F2LfsdOsLnCojz/iSH8MxUt/FD2S5XBSVPhmArFUXcpCQ2Hkiw==", + "dev": true, + "requires": { + "@sinonjs/commons": "^3.0.1" + } + }, + "@sinonjs/samsam": { + "version": "8.0.2", + "resolved": "https://registry.npmjs.org/@sinonjs/samsam/-/samsam-8.0.2.tgz", + "integrity": "sha512-v46t/fwnhejRSFTGqbpn9u+LQ9xJDse10gNnPgAcxgdoCDMXj/G2asWAC/8Qs+BAZDicX+MNZouXT1A7c83kVw==", + "dev": true, + "requires": { + "@sinonjs/commons": "^3.0.1", + "lodash.get": "^4.4.2", + "type-detect": "^4.1.0" + }, + "dependencies": { + "type-detect": { + "version": "4.1.0", + "resolved": "https://registry.npmjs.org/type-detect/-/type-detect-4.1.0.tgz", + "integrity": "sha512-Acylog8/luQ8L7il+geoSxhEkazvkslg7PSNKOX59mbB9cOveP5aq9h74Y7YU8yDpJwetzQQrfIwtf4Wp4LKcw==", + "dev": true + } + } + }, "@sovpro/delimited-stream": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/@sovpro/delimited-stream/-/delimited-stream-1.1.0.tgz", @@ -17373,6 +17499,12 @@ "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", "dev": true }, + "lodash.get": { + "version": "4.4.2", + "resolved": "https://registry.npmjs.org/lodash.get/-/lodash.get-4.4.2.tgz", + "integrity": "sha512-z+Uw/vLuy6gQe8cfaFWD7p0wVv8fJl3mbzXh33RS+0oW2wvUqiRXiQ69gLWSLpgB5/6sU+r6BlQR0MBILadqTQ==", + "dev": true + }, "lodash.merge": { "version": "4.6.2", "resolved": "https://registry.npmjs.org/lodash.merge/-/lodash.merge-4.6.2.tgz", @@ -19232,6 +19364,27 @@ "integrity": "sha512-wnD2ZE+l+SPC/uoS0vXeE9L1+0wuaMqKlfz9AMUo38JsyLSBWSFcHR1Rri62LZc12vLr1gb3jl7iwQhgwpAbGQ==", "dev": true }, + "sinon": { + "version": "20.0.0", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-20.0.0.tgz", + "integrity": "sha512-+FXOAbdnj94AQIxH0w1v8gzNxkawVvNqE3jUzRLptR71Oykeu2RrQXXl/VQjKay+Qnh73fDt/oDfMo6xMeDQbQ==", + "dev": true, + "requires": { + "@sinonjs/commons": "^3.0.1", + "@sinonjs/fake-timers": "^13.0.5", + "@sinonjs/samsam": "^8.0.1", + "diff": "^7.0.0", + "supports-color": "^7.2.0" + }, + "dependencies": { + "diff": { + "version": "7.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-7.0.0.tgz", + "integrity": "sha512-PJWHUb1RFevKCwaFA9RlG5tCd+FO5iRh9A8HEtkmBH2Li03iJriB6m6JIN4rGz3K3JLawI7/veA1xzRKP6ISBw==", + "dev": true + } + } + }, "slash": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/slash/-/slash-5.1.0.tgz", @@ -19643,6 +19796,12 @@ "prelude-ls": "^1.2.1" } }, + "type-detect": { + "version": "4.0.8", + "resolved": "https://registry.npmjs.org/type-detect/-/type-detect-4.0.8.tgz", + "integrity": "sha512-0fr/mIH1dlO+x7TlcMy+bIDqKPsw/70tVyeHW787goQjhmqaZe10uwLujubK9q9Lg6Fiho1KUKDYz0Z7k7g5/g==", + "dev": true + }, "type-fest": { "version": "4.39.1", "resolved": "https://registry.npmjs.org/type-fest/-/type-fest-4.39.1.tgz", diff --git a/package.json b/package.json index 5bb15a14..b532d492 100644 --- a/package.json +++ b/package.json @@ -64,6 +64,7 @@ "prettier-plugin-jsdoc": "^1.3.2", "prettier-plugin-multiline-arrays": "^4.0.3", "prettier-plugin-packagejson": "^2.5.10", + "sinon": "^20.0.0", "stream-match": "^1.2.1", "typescript": "^5.0.4" }, diff --git a/test/zinnia.test.js b/test/zinnia.test.js new file mode 100644 index 00000000..3ca135f0 --- /dev/null +++ b/test/zinnia.test.js @@ -0,0 +1,25 @@ +// Unit test for maybeReportErrorToSentry using dependency-injected spy. +// This avoids mocking ESM modules directly. +import assert from 'node:assert' +import sinon from 'sinon' +import { maybeReportErrorToSentry } from '../lib/zinnia.js' + +describe('maybeReportErrorToSentry', () => { + it('should NOT report error when reportToSentry is false', () => { + // Inject spy instead of using real Sentry + const spy = sinon.spy() + const error = new Error('Expected error') + error.reportToSentry = false + + maybeReportErrorToSentry(error, spy) + assert.strictEqual(spy.called, false) + }) + + it('should report error when reportToSentry is not set', () => { + const spy = sinon.spy() + const error = new Error('Unexpected error') + + maybeReportErrorToSentry(error, spy) + assert.strictEqual(spy.called, true) + }) +})