Skip to content

Commit 2a2e8cb

Browse files
JPeer264JPeer264
authored
fix(node-core): Ignore worker threads in OnUncaughtException (#18689)
closes #18592 closes [JS-1347](https://linear.app/getsentry/issue/JS-1347/sentry-changing-worker-thread-error-behavior) The [onUncaughtException integration](https://docs.sentry.io/platforms/javascript/guides/express/configuration/integrations/onuncaughtexception/) triggered for errors inside workers, which caused a wrong error code overall. Since we already have a [Child Process integration](https://docs.sentry.io/platforms/javascript/guides/express/configuration/integrations/childProcess/) which handles errors it would make most sense to disable the onUncaughtException integration entirely for workers. Another option would also be to only ignore `shouldApplyFatalHandlingLogic` for workers, which was the main reason to exit the entire process, even though the error was handled - but I don't like this approach, since the child processes errors will be handled anyways in the other integration. **Interesting finding:** When using `--import` or `--require` CLI flags, these are propagated to worker threads, so `Sentry.init()` runs twice: once in the main thread (`isMainThread === true`) and once in the worker (`isMainThread === false`). However, when using inline `require()` inside a file, it is NOT propagated to workers, so it initializes only once with `isMainThread === true`. This means the bug primarily manifests with ESM (`--import`) or when CJS uses `--require` (which may be less common). The `caught-worker-inline.js` test always passes (inline require), while `caught-worker.js` with `--require` only passes after the fix is applied, demonstrating this behavior.
1 parent b855664 commit 2a2e8cb

File tree

9 files changed

+220
-0
lines changed

9 files changed

+220
-0
lines changed

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as childProcess from 'child_process';
22
import * as path from 'path';
33
import { describe, expect, test } from 'vitest';
4+
import { conditionalTest } from '../../../utils';
45
import { createRunner } from '../../../utils/runner';
56

67
describe('OnUncaughtException integration', () => {
@@ -101,4 +102,129 @@ describe('OnUncaughtException integration', () => {
101102
.start()
102103
.completed();
103104
});
105+
106+
conditionalTest({ max: 18 })('Worker thread error handling Node 18', () => {
107+
test('should capture uncaught worker thread errors - without childProcess integration', async () => {
108+
await createRunner(__dirname, 'worker-thread/uncaught-worker.mjs')
109+
.withInstrument(path.join(__dirname, 'worker-thread/instrument.mjs'))
110+
.expect({
111+
event: {
112+
level: 'fatal',
113+
exception: {
114+
values: [
115+
{
116+
type: 'Error',
117+
value: 'job failed',
118+
mechanism: {
119+
type: 'auto.node.onuncaughtexception',
120+
handled: false,
121+
},
122+
stacktrace: {
123+
frames: expect.any(Array),
124+
},
125+
},
126+
],
127+
},
128+
},
129+
})
130+
.start()
131+
.completed();
132+
});
133+
});
134+
135+
// childProcessIntegration only exists in Node 20+
136+
conditionalTest({ min: 20 })('Worker thread error handling Node 20+', () => {
137+
test.each(['mjs', 'js'])('should not interfere with worker thread error handling ".%s"', async extension => {
138+
const runner = createRunner(__dirname, `worker-thread/caught-worker.${extension}`)
139+
.withFlags(
140+
extension === 'mjs' ? '--import' : '--require',
141+
path.join(__dirname, `worker-thread/instrument.${extension}`),
142+
)
143+
.expect({
144+
event: {
145+
level: 'error',
146+
exception: {
147+
values: [
148+
{
149+
type: 'Error',
150+
value: 'job failed',
151+
mechanism: {
152+
type: 'auto.child_process.worker_thread',
153+
handled: false,
154+
},
155+
stacktrace: {
156+
frames: expect.any(Array),
157+
},
158+
},
159+
],
160+
},
161+
},
162+
})
163+
.start();
164+
165+
await runner.completed();
166+
167+
const logs = runner.getLogs();
168+
169+
expect(logs).toEqual(expect.arrayContaining([expect.stringMatching(/^caught Error: job failed/)]));
170+
});
171+
172+
test('should not interfere with worker thread error handling when required inline', async () => {
173+
const runner = createRunner(__dirname, 'worker-thread/caught-worker-inline.js')
174+
.expect({
175+
event: {
176+
level: 'error',
177+
exception: {
178+
values: [
179+
{
180+
type: 'Error',
181+
value: 'job failed',
182+
mechanism: {
183+
type: 'auto.child_process.worker_thread',
184+
handled: false,
185+
},
186+
stacktrace: {
187+
frames: expect.any(Array),
188+
},
189+
},
190+
],
191+
},
192+
},
193+
})
194+
.start();
195+
196+
await runner.completed();
197+
198+
const logs = runner.getLogs();
199+
200+
expect(logs).toEqual(expect.arrayContaining([expect.stringMatching(/^caught Error: job failed/)]));
201+
});
202+
203+
test('should capture uncaught worker thread errors', async () => {
204+
await createRunner(__dirname, 'worker-thread/uncaught-worker.mjs')
205+
.withInstrument(path.join(__dirname, 'worker-thread/instrument.mjs'))
206+
.expect({
207+
event: {
208+
level: 'error',
209+
exception: {
210+
values: [
211+
{
212+
type: 'Error',
213+
value: 'job failed',
214+
mechanism: {
215+
type: 'auto.child_process.worker_thread',
216+
handled: false,
217+
},
218+
stacktrace: {
219+
frames: expect.any(Array),
220+
},
221+
},
222+
],
223+
},
224+
},
225+
})
226+
.start()
227+
.completed();
228+
});
229+
});
104230
});

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/caught-worker-inline.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// reuse the same worker script as the other tests
2+
// just now in one file
3+
require('./instrument.js');
4+
require('./caught-worker.js');

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/caught-worker.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
const path = require('path');
2+
const { Worker } = require('worker_threads');
3+
4+
function runJob() {
5+
const worker = new Worker(path.join(__dirname, 'job.js'));
6+
return new Promise((resolve, reject) => {
7+
worker.once('error', reject);
8+
worker.once('exit', code => {
9+
if (code) reject(new Error(`Worker exited with code ${code}`));
10+
else resolve();
11+
});
12+
});
13+
}
14+
15+
runJob()
16+
.then(() => {
17+
// eslint-disable-next-line no-console
18+
console.log('Job completed successfully');
19+
})
20+
.catch(err => {
21+
// eslint-disable-next-line no-console
22+
console.error('caught', err);
23+
});

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/caught-worker.mjs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import path from 'path';
2+
import { Worker } from 'worker_threads';
3+
4+
const __dirname = new URL('.', import.meta.url).pathname;
5+
6+
function runJob() {
7+
const worker = new Worker(path.join(__dirname, 'job.js'));
8+
return new Promise((resolve, reject) => {
9+
worker.once('error', reject);
10+
worker.once('exit', code => {
11+
if (code) reject(new Error(`Worker exited with code ${code}`));
12+
else resolve();
13+
});
14+
});
15+
}
16+
17+
try {
18+
await runJob();
19+
// eslint-disable-next-line no-console
20+
console.log('Job completed successfully');
21+
} catch (err) {
22+
// eslint-disable-next-line no-console
23+
console.error('caught', err);
24+
}

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/instrument.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const Sentry = require('@sentry/node');
2+
const { loggingTransport } = require('@sentry-internal/node-integration-tests');
3+
4+
Sentry.init({
5+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
6+
tracesSampleRate: 0,
7+
debug: false,
8+
transport: loggingTransport,
9+
});

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/instrument.mjs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import * as Sentry from '@sentry/node';
2+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
3+
4+
Sentry.init({
5+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
6+
tracesSampleRate: 0,
7+
debug: false,
8+
transport: loggingTransport,
9+
});

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/job.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
throw new Error('job failed');

‎dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/worker-thread/uncaught-worker.mjs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import path from 'path';
2+
import { Worker } from 'worker_threads';
3+
4+
const __dirname = new URL('.', import.meta.url).pathname;
5+
6+
function runJob() {
7+
const worker = new Worker(path.join(__dirname, 'job.js'));
8+
return new Promise((resolve, reject) => {
9+
worker.once('error', reject);
10+
worker.once('exit', code => {
11+
if (code) reject(new Error(`Worker exited with code ${code}`));
12+
else resolve();
13+
});
14+
});
15+
}
16+
17+
await runJob();

‎packages/node-core/src/integrations/onuncaughtexception.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { captureException, debug, defineIntegration, getClient } from '@sentry/core';
2+
import { isMainThread } from 'worker_threads';
23
import { DEBUG_BUILD } from '../debug-build';
34
import type { NodeClient } from '../sdk/client';
45
import { logAndExitProcess } from '../utils/errorhandling';
@@ -44,6 +45,12 @@ export const onUncaughtExceptionIntegration = defineIntegration((options: Partia
4445
return {
4546
name: INTEGRATION_NAME,
4647
setup(client: NodeClient) {
48+
// errors in worker threads are already handled by the childProcessIntegration
49+
// also we don't want to exit the Node process on worker thread errors
50+
if (!isMainThread) {
51+
return;
52+
}
53+
4754
global.process.on('uncaughtException', makeErrorHandler(client, optionsWithDefaults));
4855
},
4956
};

0 commit comments

Comments
 (0)