diff --git a/MIGRATION.md b/MIGRATION.md index fd633ac0d9ea..91a9ea8bd6dc 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -97,6 +97,24 @@ Affected SDKs: `@sentry/node` and all dependents. The new channel-based instrumentations (using `orchestrion` instead of `import-in-the-middle`) are now the default. They were available opt-in in v10. This unlocks instrumenting at run and build time, which enables instrumentation at deployment targets like Vercel and Netlify, as well as using instrumentations on non-Node runtimes like Cloudflare, Bun and Deno. For most users this requires no changes. +### Initializing via `--require` is no longer supported + +Affected SDKs: `@sentry/node` and all dependents. + +Node re-runs `--require` preloads on the internal module loader thread it spawns for `Module.register()` — which the SDK triggers itself when it installs its instrumentation hooks. A `--require`d instrument file therefore ran `Sentry.init()` a second time, on a thread that never executes any of your code. The SDK now skips initialization on that thread and warns when it detects that it was loaded through `--require`. + +Use [`--import`](https://nodejs.org/api/cli.html#--importmodule) instead. It is not re-run on the loader thread, and it works for CommonJS apps too — the instrument file's extension (`.cjs`, or `.js` in a package without `"type": "module"`) is what decides that it loads as CommonJS: + +```bash +# Before +node --require ./instrument.js app.js + +# After +node --import ./instrument.js app.js +``` + +The same applies to the no-code entry points, e.g. `node --import=@sentry/node/init app.js` and `node --import @sentry/node/preload app.js`. + ### Span streaming is now the default Affected SDKs: All SDKs. diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2-non-vite/package.json b/dev-packages/e2e-tests/test-applications/create-remix-app-v2-non-vite/package.json index 38a7e231ddf1..4cf602df06dc 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2-non-vite/package.json +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2-non-vite/package.json @@ -4,7 +4,7 @@ "scripts": { "build": "remix build", "dev": "remix dev", - "start": "NODE_OPTIONS='--require=./instrument.server.cjs' remix-serve build/index.js", + "start": "NODE_OPTIONS='--import=./instrument.server.cjs' remix-serve build/index.js", "typecheck": "tsc", "clean": "npx rimraf node_modules pnpm-lock.yaml", "test:build": "pnpm install && npx playwright install && pnpm build", diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/package.json b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/package.json index 978f2abbd4d7..a60ba33a6f6a 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/package.json +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/package.json @@ -5,7 +5,7 @@ "scripts": { "build": "remix vite:build && pnpm typecheck", "dev": "remix vite:dev", - "start": "NODE_OPTIONS='--require=./instrument.server.cjs' remix-serve build/server/index.js", + "start": "NODE_OPTIONS='--import=./instrument.server.cjs' remix-serve build/server/index.js", "typecheck": "tsc", "clean": "npx rimraf node_modules pnpm-lock.yaml", "test:build": "pnpm install && pnpm build", diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/build-injection.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/build-injection.test.ts index cfa27164a765..714cfd5e9d6e 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/build-injection.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/build-injection.test.ts @@ -4,7 +4,7 @@ import { expect, test } from '@playwright/test'; // The `db.test.ts` runtime assertions prove orchestrion spans appear, but spans alone // don't prove they came from the BUILD-time transform: if the Vite plugin silently -// failed to load, the deps would stay external and the runtime `--require` hook would +// failed to load, the deps would stay external and the runtime `--import` hook would // inject the channels at runtime instead - the span tests would still pass. These // assertions inspect the built server bundle directly so a broken plugin can't hide // behind that runtime fallback. Only relevant in the orchestrion variant. diff --git a/dev-packages/e2e-tests/test-applications/node-express-cjs-preload/package.json b/dev-packages/e2e-tests/test-applications/node-express-cjs-preload/package.json index 125372c4501a..c2bad813c24e 100644 --- a/dev-packages/e2e-tests/test-applications/node-express-cjs-preload/package.json +++ b/dev-packages/e2e-tests/test-applications/node-express-cjs-preload/package.json @@ -3,7 +3,7 @@ "version": "1.0.0", "private": true, "scripts": { - "start": "node --require @sentry/node/preload src/app.js", + "start": "node --import @sentry/node/preload src/app.js", "clean": "npx rimraf node_modules pnpm-lock.yaml", "test:build": "pnpm install", "test:assert": "playwright test" diff --git a/dev-packages/e2e-tests/test-applications/remix-server-timing/package.json b/dev-packages/e2e-tests/test-applications/remix-server-timing/package.json index fd57d2920d5a..2db256acfd99 100644 --- a/dev-packages/e2e-tests/test-applications/remix-server-timing/package.json +++ b/dev-packages/e2e-tests/test-applications/remix-server-timing/package.json @@ -4,7 +4,7 @@ "scripts": { "build": "remix vite:build && pnpm typecheck", "dev": "remix vite:dev", - "start": "NODE_OPTIONS='--require=./instrument.server.cjs' remix-serve build/server/index.js", + "start": "NODE_OPTIONS='--import=./instrument.server.cjs' remix-serve build/server/index.js", "typecheck": "tsc", "clean": "npx rimraf node_modules pnpm-lock.yaml", "test:build": "pnpm install && pnpm build", diff --git a/dev-packages/node-integration-tests/suites/no-code/app.js b/dev-packages/node-integration-tests/suites/no-code/app.js deleted file mode 100644 index cb1937007297..000000000000 --- a/dev-packages/node-integration-tests/suites/no-code/app.js +++ /dev/null @@ -1,3 +0,0 @@ -setTimeout(() => { - throw new Error('Test error'); -}, 1000); diff --git a/dev-packages/node-integration-tests/suites/no-code/test.ts b/dev-packages/node-integration-tests/suites/no-code/test.ts index dfe58ff03f72..629729373e10 100644 --- a/dev-packages/node-integration-tests/suites/no-code/test.ts +++ b/dev-packages/node-integration-tests/suites/no-code/test.ts @@ -17,15 +17,6 @@ describe('no-code init', () => { cleanupChildProcesses(); }); - test('CJS', async () => { - await createRunner(__dirname, 'app.js') - .withFlags('--require=@sentry/node/init') - .withMockSentryServer() - .expect({ event: EVENT }) - .start() - .completed(); - }); - describe('--import', () => { test('ESM', async () => { await createRunner(__dirname, 'app.mjs') diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-instrument.js b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-instrument.cjs similarity index 100% rename from dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-instrument.js rename to dev-packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-instrument.cjs diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts index b0802b96f263..9acc9ea0d92a 100644 --- a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -91,11 +91,11 @@ module.exports = { out_of_app_function };`, .completed(); }); - test('Should include local variables when instrumenting via --require', async () => { - const requirePath = path.resolve(__dirname, 'local-variables-instrument.js'); + test('Should include local variables when instrumenting via --import', async () => { + const instrumentPath = path.resolve(__dirname, 'local-variables-instrument.cjs'); await createRunner(__dirname, 'local-variables-no-sentry.js') - .withFlags(`--require=${requirePath}`) + .withFlags(`--import=${instrumentPath}`) .expect({ event: EXPECTED_LOCAL_VARIABLES_EVENT }) .start() .completed(); diff --git a/dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts b/dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts index 10981a84d103..a7c8ce53e3b5 100644 --- a/dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts @@ -136,10 +136,7 @@ describe('OnUncaughtException integration', () => { conditionalTest({ min: 20 })('Worker thread error handling Node 20+', () => { test.each(['mjs', 'js'])('should not interfere with worker thread error handling ".%s"', async extension => { const runner = createRunner(__dirname, `worker-thread/caught-worker.${extension}`) - .withFlags( - extension === 'mjs' ? '--import' : '--require', - path.join(__dirname, `worker-thread/instrument.${extension}`), - ) + .withFlags('--import', path.join(__dirname, `worker-thread/instrument.${extension}`)) .expect({ event: { level: 'error', diff --git a/dev-packages/node-integration-tests/suites/tracing/mysql/test.ts b/dev-packages/node-integration-tests/suites/tracing/mysql/test.ts index 63fe04d744b6..9cd0d93fca64 100644 --- a/dev-packages/node-integration-tests/suites/tracing/mysql/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/mysql/test.ts @@ -66,7 +66,7 @@ describe('mysql auto instrumentation', () => { // Each case maps to one of the two documented use cases, in opt-in and // non-opt-in form. `flags` are extra Node CLI flags; the instrument file is - // always loaded via `--import` (esm) / `--require` (cjs) by the runner. + // always loaded via `--import` by the runner. const CASES = [ // OpenTelemetry default — no opt-in, no injection. (OTel does not support ESM.) { label: 'opentelemetry (default)', env: {}, flags: [], origin: undefined, failsOnEsm: true }, diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/instrument-orchestrion.mjs b/dev-packages/node-integration-tests/suites/tracing/postgres/instrument-orchestrion.mjs index d0ac1aec0b2c..74c8da26296b 100644 --- a/dev-packages/node-integration-tests/suites/tracing/postgres/instrument-orchestrion.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/instrument-orchestrion.mjs @@ -1,6 +1,6 @@ // Opting in via `experimentalUseDiagnosticsChannelInjection()` before `init()` // is all that's needed. Because this file is loaded -// (via `--import`/`--require`) before the scenario imports `pg`, +// (via `--import`) before the scenario imports `pg`, // `Sentry.init()` synchronously installs the channel-injection hooks, so the // OTel `Postgres` instrumentation is swapped for the diagnostics-channel one. import * as Sentry from '@sentry/node'; diff --git a/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/instrument.ts b/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/instrument.cjs similarity index 54% rename from dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/instrument.ts rename to dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/instrument.cjs index 617cbbff351a..0f7c1b08fcbd 100644 --- a/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/instrument.ts +++ b/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/instrument.cjs @@ -1,5 +1,5 @@ -import * as Sentry from '@sentry/node'; -import { loggingTransport } from '@sentry-internal/node-integration-tests'; +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', diff --git a/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.ts b/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.js similarity index 68% rename from dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.ts rename to dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.js index ca9ebc62f13e..3ecc21e50159 100644 --- a/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.ts +++ b/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.js @@ -1,6 +1,6 @@ -import * as Sentry from '@sentry/node'; -import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; -import express from 'express'; +const Sentry = require('@sentry/node'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); +const express = require('express'); const app = express(); diff --git a/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/test.ts index 9ccb451ab3b2..736fdb7857ca 100644 --- a/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/test.ts @@ -10,8 +10,8 @@ test('errors and transactions get a unique traceId per request, when tracing is const eventTraceIds: string[] = []; const transactionTraceIds: string[] = []; - const runner = createRunner(__dirname, 'server.ts') - .withFlags('--require', join(__dirname, 'instrument.ts')) + const runner = createRunner(__dirname, 'server.js') + .withFlags('--import', join(__dirname, 'instrument.cjs')) .expect({ event: event => { eventTraceIds.push(event.contexts?.trace?.trace_id || ''); diff --git a/dev-packages/node-integration-tests/utils/runner/createEsmAndCjsTests.ts b/dev-packages/node-integration-tests/utils/runner/createEsmAndCjsTests.ts index e480bbc1b68f..c123339629c4 100644 --- a/dev-packages/node-integration-tests/utils/runner/createEsmAndCjsTests.ts +++ b/dev-packages/node-integration-tests/utils/runner/createEsmAndCjsTests.ts @@ -241,7 +241,10 @@ function prepareTmpDir( const cjsScenarioPath = join(tmpDirPath, esmScenarioBasename.replace('.mjs', '.cjs')); const cjsInstrumentPath = join(tmpDirPath, esmInstrumentBasename.replace('.mjs', '.cjs')); - const cjsFlags: string[] = ['--require', cjsInstrumentPath]; + // `--import` for both: the `.cjs` extension is what makes Node load the instrument file (and + // therefore the SDK) as CommonJS. `--require` would additionally re-run the preload on Node's + // module loader thread, which the SDK no longer supports. + const cjsFlags: string[] = ['--import', cjsInstrumentPath]; const esmFlags: string[] = ['--import', esmInstrumentPathForRun]; async function createTmpDir(): Promise { @@ -274,7 +277,7 @@ experimentalUseDiagnosticsChannelInjection();`, ); esmFlags.unshift('--import', mjsInjectOrchetrionPath); - cjsFlags.unshift('--require', cjsInjectOrchetrionPath); + cjsFlags.unshift('--import', cjsInjectOrchetrionPath); } // Copy any additional files/dirs into tmp dir diff --git a/dev-packages/node-integration-tests/utils/runner/createRunner.ts b/dev-packages/node-integration-tests/utils/runner/createRunner.ts index 2b7cbd2b0c84..c3a1979ad92f 100644 --- a/dev-packages/node-integration-tests/utils/runner/createRunner.ts +++ b/dev-packages/node-integration-tests/utils/runner/createRunner.ts @@ -108,6 +108,12 @@ const NODE_MAJOR = Number(process.versions.node.split('.')[0]); const COMPILE_CACHE_ENV: Record = NODE_MAJOR >= 22 ? { NODE_COMPILE_CACHE: join(tmpdir(), 'sentry-node-it-compile-cache') } : {}; +/** Node flags that preload a module before the entry point. */ +const PRELOAD_FLAGS = ['--import', '--require', '-r']; + +/** tsx's CommonJS require hook, preloaded for `.ts` scenarios. */ +const TS_LOADER = 'tsx/cjs'; + export const CLEANUP_STEPS = new Set(); export function cleanupChildProcesses(): void { @@ -147,7 +153,7 @@ export function createRunner(...paths: string[]) { // 22+ gives the scenario a different `@sentry/node` instance than the CJS instrument/auto-flush, // so instrumentation and flushing target the wrong SDK object. The require hook keeps one CJS // instance, matching how ts-node loaded them. - flags.push('-r', 'tsx/cjs'); + flags.push('-r', TS_LOADER); } // Cleanup steps registered by this specific runner (child process, docker, mock server). They are @@ -417,11 +423,9 @@ export function createRunner(...paths: string[]) { // flush keeps the event loop alive until queued envelopes reach the // transport, then the process exits naturally. // - // We inject the matching loader for the scenario's module system - // (detected by whether `flags` already contains `--import` for the - // instrument file). For ESM scenarios we use `--import auto-flush.mjs` - // so the `import * as Sentry` resolves to the same SDK instance the - // scenario uses; for CJS we use `--require auto-flush.cjs`. + // We inject the loader matching the scenario's module system, so that its + // `Sentry` reference resolves to the same SDK instance the scenario uses — + // see `buildAutoFlushFlags`. // // Skipped when no envelopes are expected — these tests (e.g. ANR // `should-exit`, `ensureNoErrorOutput`) verify the child exits @@ -429,7 +433,7 @@ export function createRunner(...paths: string[]) { // requests to the fake DSN. const wantsAutoFlush = !ensureNoErrorOutput && (expectedEnvelopes.length > 0 || (expectedEnvelopeHeaders?.length ?? 0) > 0); - const childFlags = wantsAutoFlush ? [...buildAutoFlushFlags(flags), ...flags] : flags; + const childFlags = wantsAutoFlush ? [...buildAutoFlushFlags(flags, testPath), ...flags] : flags; child = spawn('node', [...childFlags, testPath], { env }); @@ -651,25 +655,59 @@ function log(...args: unknown[]): void { console.log(...args.map(arg => normalize(arg))); } +/** + * Extracts the preloaded module paths from Node flags, accepting both the + * two-element form (`--import foo`, e.g. `withInstrument`) and the single-element + * form (`--import=foo`, e.g. `withFlags('--import=@sentry/node/init')` in + * `suites/no-code/test.ts`). + */ +function getPreloadPaths(flags: readonly string[]): string[] { + const paths: string[] = []; + + for (let i = 0; i < flags.length; i++) { + const flag = flags[i] as string; + const [name, ...rest] = flag.split('='); + + if (!PRELOAD_FLAGS.includes(name as string)) { + continue; + } + + const path = rest.length ? rest.join('=') : flags[++i]; + if (path) { + paths.push(path); + } + } + + return paths; +} + /** * Returns Node flags that inject the auto-flush loader matching the scenario's - * module system. ESM scenarios already have `--import` for the instrument - * file — we mirror that with `--import auto-flush.mjs` so both resolve to the - * same `@sentry/node` instance. Otherwise we fall back to `--require - * auto-flush.cjs`. + * module system. + * + * Which of the two SDK builds (CJS or ESM) holds the queued envelopes is decided by + * the file that calls `Sentry.init()` — the last preloaded instrument file if there is + * one, otherwise the scenario itself. Getting this wrong is silent: the flush targets + * the other build's client, which has nothing queued, and no envelope ever arrives. * - * Node accepts both `--import foo` (two array elements, e.g. `withInstrument` - * or `withFlags('--import', foo)`) and `--import=foo` (one element, e.g. - * `withFlags('--import=@sentry/node/init')` in `suites/no-code/test.ts`); we - * have to recognise both, otherwise the missed form silently gets - * `auto-flush.cjs` and the flush targets the wrong SDK instance. + * The flag name is no longer a usable signal, since CJS instrument files are preloaded + * with `--import` too (`--require` re-runs the preload on Node's module loader thread). + * The extension is: `.mjs` and extensionless package specifiers such as + * `@sentry/node/init` resolve as ESM, while `.cjs`, `.js` and `.ts` are all CommonJS + * here because the test package sets no `"type"`. + * + * The CJS loader stays on `--require`: any `--import` makes Node resolve the entry point + * through the ESM loader, which rejects the `.ts` scenarios that `tsx/cjs` handles. Unlike + * an instrument file it never calls `Sentry.init()`, so the loader thread is not a concern. */ -function buildAutoFlushFlags(existingFlags: readonly string[]): string[] { - const isEsm = existingFlags.some(flag => flag === '--import' || flag.startsWith('--import=')); - if (isEsm) { - return ['--import', join(__dirname, 'auto-flush.mjs')]; - } - return ['--require', join(__dirname, 'auto-flush.cjs')]; +function buildAutoFlushFlags(existingFlags: readonly string[], testPath: string): string[] { + const initPath = + getPreloadPaths(existingFlags) + .filter(path => path !== TS_LOADER) + .at(-1) ?? testPath; + const isEsm = initPath.endsWith('.mjs') || !/\.[cm]?[jt]s$/.test(initPath); + + return isEsm ? ['--import', join(__dirname, 'auto-flush.mjs')] : ['--require', join(__dirname, 'auto-flush.cjs')]; } function expectErrorEvent(item: Event, expected: ExpectedEvent): void { diff --git a/packages/node/src/init.ts b/packages/node/src/init.ts index 3d4ba2ceff90..acedff109ce1 100644 --- a/packages/node/src/init.ts +++ b/packages/node/src/init.ts @@ -1,7 +1,7 @@ import { init } from './sdk'; /** - * The @sentry/node/init export can be used with the node --import and --require args to initialize the SDK entirely via + * The @sentry/node/init export can be used with the node --import arg to initialize the SDK entirely via * environment variables. * * > SENTRY_DSN=https://examplePublicKey@o0.ingest.sentry.io/0 SENTRY_TRACES_SAMPLE_RATE=1.0 node --import=@sentry/node/init app.mjs diff --git a/packages/node/src/preload.ts b/packages/node/src/preload.ts index 7684449ee524..575a0248ad76 100644 --- a/packages/node/src/preload.ts +++ b/packages/node/src/preload.ts @@ -7,7 +7,7 @@ const integrationsStr = process.env.SENTRY_PRELOAD_INTEGRATIONS; const integrations = integrationsStr ? integrationsStr.split(',').map(integration => integration.trim()) : undefined; /** - * The @sentry/node/preload export can be used with the node --import and --require args to preload the OTEL + * The @sentry/node/preload export can be used with the node --import arg to preload the OTEL * instrumentation, without initializing the Sentry SDK. * * This is useful if you cannot initialize the SDK immediately, but still want to preload the instrumentation, diff --git a/packages/node/src/sdk/index.ts b/packages/node/src/sdk/index.ts index a3058725d19b..418603c28ab5 100644 --- a/packages/node/src/sdk/index.ts +++ b/packages/node/src/sdk/index.ts @@ -21,6 +21,7 @@ import { setOpenTelemetryContextAsyncContextStrategy, setupEventContextTrace, } from '@sentry/opentelemetry'; +import { isMainThread, parentPort } from 'node:worker_threads'; import { DEBUG_BUILD } from '../debug-build'; import { childProcessIntegration } from '../integrations/childProcess'; import { consoleIntegration } from '../integrations/console'; @@ -38,6 +39,7 @@ import { systemErrorIntegration } from '../integrations/systemError'; import { getAutoPerformanceIntegrations } from '../integrations/tracing'; import { makeNodeTransport } from '../transports'; import type { NodeClientOptions, NodeOptions } from '../types'; +import { getEntryPointType } from '../utils/entry-point'; import { getSpotlightConfig } from '../utils/spotlight'; import { defaultStackParser, getSentryRelease } from './api'; import { NodeClient } from './client'; @@ -150,6 +152,24 @@ function _init( options: NodeOptions | undefined = {}, getDefaultIntegrationsImpl: (options: Options) => Integration[], ): NodeClient | undefined { + // Node re-runs `--require` preloads (though not `--import` ones) on the module loader thread it + // spawns for `Module.register()`, which `init()` itself triggers (channel injection, the ESM + // loader hook). So a `--require`d instrument file re-enters `init()` there, on a thread that + // never runs app code. It is recognizable as the only thread without a `parentPort`: + // user-created workers always have one and are legitimately instrumented. + if (!isMainThread && !parentPort) { + return undefined; + } + + if (getEntryPointType() === 'require') { + consoleSandbox(() => { + // eslint-disable-next-line no-console + console.warn( + '[Sentry] Initializing the SDK via the Node `--require` flag is no longer supported, because Node re-runs `--require` preloads on its module loader thread. Use `--import` instead: `node --import ./instrument.js app.js`', + ); + }); + } + applySdkMetadata(options, 'node'); // Resolve the tracing-affecting options (e.g. `SENTRY_TRACES_SAMPLE_RATE`) up front so that both diff --git a/packages/node/src/utils/entry-point.ts b/packages/node/src/utils/entry-point.ts index e73130f1efff..56745334f746 100644 --- a/packages/node/src/utils/entry-point.ts +++ b/packages/node/src/utils/entry-point.ts @@ -23,7 +23,7 @@ export function parseProcessPaths(proc: ProcessInterface): ProcessArgs { const joinedArgs = execArgv.join(' '); const importPaths = Array.from(joinedArgs.matchAll(/--import[ =](\S+)/g)).map(e => resolve(cwd, e[1] || '')); - const requirePaths = Array.from(joinedArgs.matchAll(/--require[ =](\S+)/g)).map(e => resolve(cwd, e[1] || '')); + const requirePaths = Array.from(joinedArgs.matchAll(/(?:--require|-r)[ =](\S+)/g)).map(e => resolve(cwd, e[1] || '')); return { appPath, importPaths, requirePaths }; } diff --git a/packages/node/test/sdk/init-preload.test.ts b/packages/node/test/sdk/init-preload.test.ts new file mode 100644 index 000000000000..e0b92bf21c5e --- /dev/null +++ b/packages/node/test/sdk/init-preload.test.ts @@ -0,0 +1,89 @@ +import { originalConsoleMethods } from '@sentry/core'; +import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from 'vitest'; +import { getClient } from '../../src/'; +import { init } from '../../src/sdk'; +import { cleanupOtel } from '../helpers/mockSdkInit'; +import type * as WorkerThreads from 'node:worker_threads'; + +// eslint-disable-next-line no-var +declare var global: any; + +const PUBLIC_DSN = 'https://username@domain/123'; + +const thread = { isMainThread: true, parentPort: {} as unknown }; +let entryPointType = 'app'; + +vi.mock('node:worker_threads', async importOriginal => ({ + ...(await importOriginal()), + get isMainThread() { + return thread.isMainThread; + }, + get parentPort() { + return thread.parentPort; + }, +})); + +vi.mock('../../src/utils/entry-point', () => ({ + getEntryPointType: () => entryPointType, +})); + +/** + * Once the console integration has run, `consoleSandbox` swaps `console.warn` for the native + * method it stashed in `originalConsoleMethods`, bypassing a plain `console` spy. Cover both. + */ +function spyOnConsoleWarn(): Mock { + const spy = vi.fn(); + + vi.spyOn(console, 'warn').mockImplementation(spy); + if (originalConsoleMethods.warn) { + vi.spyOn(originalConsoleMethods, 'warn').mockImplementation(spy); + } + + return spy; +} + +describe('init() preload guards', () => { + beforeEach(() => { + global.__SENTRY__ = {}; + thread.isMainThread = true; + thread.parentPort = {}; + entryPointType = 'app'; + }); + + afterEach(() => { + cleanupOtel(); + vi.clearAllMocks(); + }); + + it('skips initialization on the module loader thread', () => { + thread.isMainThread = false; + thread.parentPort = undefined; + + expect(init({ dsn: PUBLIC_DSN })).toBeUndefined(); + expect(getClient()).toBeUndefined(); + }); + + it('initializes on user-created worker threads', () => { + thread.isMainThread = false; + + expect(init({ dsn: PUBLIC_DSN })).toBeDefined(); + }); + + it('warns when initialized from a `--require` preload', () => { + const warnSpy = spyOnConsoleWarn(); + entryPointType = 'require'; + + init({ dsn: PUBLIC_DSN }); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Use `--import` instead')); + }); + + it('does not warn when initialized from an `--import` preload', () => { + const warnSpy = spyOnConsoleWarn(); + entryPointType = 'import'; + + init({ dsn: PUBLIC_DSN }); + + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/node/test/utils/entry-point.test.ts b/packages/node/test/utils/entry-point.test.ts index bd65ca5506c0..118657a38743 100644 --- a/packages/node/test/utils/entry-point.test.ts +++ b/packages/node/test/utils/entry-point.test.ts @@ -43,6 +43,18 @@ const PROCESS_ARG_TESTS: [ProcessInterface, ProcessArgs][] = [ requirePaths: ['/user/tim/docs/here/something.js'], }, ], + [ + { + cwd: () => '/user/tim/docs', + argv: ['/bin/node', 'app.js'], + execArgv: ['-r', './something.js'], + }, + { + appPath: '/user/tim/docs/app.js', + importPaths: [], + requirePaths: ['/user/tim/docs/something.js'], + }, + ], ]; describe('getEntryPointType', () => { diff --git a/packages/server-utils/src/orchestrion/runtime/register.ts b/packages/server-utils/src/orchestrion/runtime/register.ts index bd559b2a6e48..063dd80469ba 100644 --- a/packages/server-utils/src/orchestrion/runtime/register.ts +++ b/packages/server-utils/src/orchestrion/runtime/register.ts @@ -2,7 +2,7 @@ import { debug, GLOBAL_OBJ } from '@sentry/core'; import { createRequire } from 'node:module'; import * as Module from 'node:module'; import { pathToFileURL } from 'node:url'; -import { isMainThread, MessageChannel, parentPort } from 'node:worker_threads'; +import { MessageChannel } from 'node:worker_threads'; import { SENTRY_INSTRUMENTATIONS } from '../config'; import type { register } from 'node:module'; import type { InstrumentationConfig } from '..'; @@ -63,18 +63,6 @@ function hasStableSyncModuleHooks(denoVersionString: string | undefined): boolea * the channel-based integrations subscribe to. */ export function registerDiagnosticsChannelInjection(options?: RegisterDiagnosticsChannelInjectionOptions): void { - // Skip Node's internal loader (hooks) threads, recognizable as the only threads without a - // `parentPort`. Node re-runs `--require` preloads (though not `--import` ones) on the loader - // thread it spawns for `Module.register()`, so this function runs there too — but that thread - // never executes app code, and its `register()` implementation (the in-thread `Hooks` class) - // has no `transferList` parameter: our transfer array lands in its `isInternal` parameter and - // Node crashes trying to load the hook as an internal builtin. User-created workers always - // have a `parentPort` and register through `CustomizedModuleLoader`, which handles - // `transferList` correctly, so they proceed and get their own instrumented loader. - if (!isMainThread && !parentPort) { - return; - } - if (GLOBAL_OBJ?.__SENTRY_ORCHESTRION__?.runtime) { return; }