Skip to content

Commit 68a3af5

Browse files
authored
fix(node): Don't recurse in logAndExitProcess on a broken stdio pipe (#24351)
A broken stdio pipe crashed Node with an OOM, fixed by making `logAndExitProcess` now guards against re-entry. I added a broken pipe test that reproduces the issue before the fix. closes #24337
1 parent 4c64ad6 commit 68a3af5

3 files changed

Lines changed: 41 additions & 0 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
const Sentry = require('@sentry/node');
2+
3+
// Unreachable rather than invalid, so `client.close()` is still pending while the
4+
// broken pipe keeps erroring.
5+
Sentry.init({
6+
dsn: 'https://public@127.0.0.1:1/1337',
7+
});
8+
9+
// The test runner closes both stdio streams, so this write raises EPIPE. Node ignores
10+
// SIGPIPE, so it arrives as an uncaught exception.
11+
setInterval(() => process.stdout.write('x'.repeat(4096)), 0);

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,26 @@ describe('OnUncaughtException integration', () => {
4747
});
4848
}));
4949

50+
test('should exit rather than recurse when stderr is a broken pipe', async () => {
51+
const testScriptPath = path.resolve(__dirname, 'broken-stdio-pipe-test-script.js');
52+
53+
// The heap cap makes a regression fail in ~1s; at the default size it takes a minute
54+
// and just looks like a hang.
55+
const child = childProcess.spawn(process.execPath, ['--max-old-space-size=64', testScriptPath], {
56+
stdio: ['ignore', 'pipe', 'pipe'],
57+
});
58+
59+
child.stdout.destroy();
60+
child.stderr.destroy();
61+
62+
const exited = await new Promise<{ code: number | null; signal: string | null }>(resolve => {
63+
child.on('exit', (code, signal) => resolve({ code, signal }));
64+
});
65+
66+
// Unbounded recursion shows up as SIGABRT from the V8 OOM abort.
67+
expect(exited).toEqual({ code: 1, signal: null });
68+
});
69+
5070
describe('with `exitEvenIfOtherHandlersAreRegistered` set to false', () => {
5171
test('should close process on uncaught error with no additional listeners registered', () =>
5272
new Promise<void>(done => {

‎packages/node/src/utils/errorhandling.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,19 @@ import type { NodeClient } from '../sdk/client';
44

55
const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
66

7+
let isShuttingDown = false;
8+
79
/**
810
* @hidden
911
*/
1012
export function logAndExitProcess(error: unknown): void {
13+
// A broken stderr makes the console write below raise EPIPE, re-entering here. Return
14+
// rather than exit, so the in-flight `client.close()` still flushes the fatal event.
15+
if (isShuttingDown) {
16+
return;
17+
}
18+
isShuttingDown = true;
19+
1120
consoleSandbox(() => {
1221
// eslint-disable-next-line no-console
1322
console.error(error);
@@ -33,6 +42,7 @@ export function logAndExitProcess(error: unknown): void {
3342
},
3443
error => {
3544
DEBUG_BUILD && debug.error(error);
45+
global.process.exit(1);
3646
},
3747
);
3848
}

0 commit comments

Comments
 (0)