Skip to content

fix: restore Sentry scope isolation when OpenTelemetry is enabled (AI-3346) - #16

Merged
benminer merged 2 commits into
mainfrom
bishkek-v1
Jul 29, 2026
Merged

fix: restore Sentry scope isolation when OpenTelemetry is enabled (AI-3346)#16
benminer merged 2 commits into
mainfrom
bishkek-v1

Conversation

@andybevan-scope3

Copy link
Copy Markdown
Contributor

Fixes cross-request leakage of tags, user identity and transaction names. Live in prod since 2026-05-26 and measured leaking customer and user identity between requests.

Linear: AI-3346

Root cause

initializeSentry passes skipOpenTelemetrySetup: config.enableOtel, which skips initOpenTelemetry() (@sentry/node/build/cjs/sdk/index.js:57). That is the only place the SDK installs SentryContextManager. Meanwhile @sentry/node-core's init calls setOpenTelemetryContextAsyncContextStrategy() unconditionally (sdk/index.js:94).

So Sentry's scope functions were backed by the OTel context, with nothing ever writing Sentry's scopes onto that context. Every withIsolationScope / getCurrentScope resolved to the process-global default scope.

What this changes

  1. provider.register() installs SentryContextManager. This is the isolation fix.
  2. attachTraceProviderForShutdownFlush(). The same skipped setup leaves client.traceProvider unassigned, and that is the only handle NodeClient.flush()/.close() have on the span processors, so graceful shutdown was discarding whatever the OTLP batch held.
  3. warnIfScopeIsolationInactive(). setGlobalContextManager refuses to replace an existing manager and reports the refusal only on OTel's diag channel.

Why (2) is not a one-line assignment

The naive client.traceProvider = provider is a regression, which is why it carries two extra pieces:

  • BasicTracerProvider.forceFlush() rejects with an array of errors if any processor fails or times out (sdk-trace-base/BasicTracerProvider.js:74-85), and NodeClient.flush() awaits it uncaught. With an unreachable collector at SIGTERM, await Sentry.close(2000) would reject before super.close() ran, silently discarding buffered Sentry error events. Export failures are now contained.
  • The OTel default forceFlushTimeoutMillis is 30s, matching this provider's exportTimeoutMillis, and the caller's flush(timeout) is not passed through. That can outlast a K8s termination grace period. Now bounded to 500ms, matching what Sentry's own setupOtel picks.
  • Skipped when client.traceProvider is already set: if a consumer completed its own OTel setup first, our register() was silently refused and our provider will never see a span, so overwriting theirs would point shutdown at a dead provider.

Why the guard is hand-rolled

Both SDK validators were tried and read from installed source. validateOpenTelemetrySetup() early-returns unless DEBUG_BUILD and reports via core.debug.error, silent unless debug: true. openTelemetrySetupCheck() calls setIsSetup() from element constructors, so it reports SentryContextManager as present merely because one was constructed, and keeps setupElements per module instance. The guard compares scope identity instead, which is exact.

Verification

Every assertion was checked by removing the fix and confirming the right tests fail, on a clean npm ci install with Node 24.

step result
full suite 55 passed
contextManager removed 4 failed
traceProvider assignment removed 2 failed
export-failure containment removed 1 failed, promise rejected "[ …(1) ]"
typecheck / biome / build clean (2 biome warnings in config.ts pre-existing)

settles the client flush when span export fails runs in ~510ms, which is the 500ms bound doing its job rather than the 30s default.

Incidental: package-lock.json was stale on main (still @scope3data/observability-js at 2.0.0) and is regenerated here. @opentelemetry/context-async-hooks moves to an explicit devDependency; the guard test needs the scope-unaware manager that SentryContextManager wraps, and relying on it transitively would break on a dedupe.

Decisions I want a second opinion on

  • Version 2.2.0. Strict semver says 2.1.1. Minor argues consumers see a real behavior change. There is a case this is closer to a major: SentryContextManager.with() clones the current scope, so a Sentry.setTag() inside a startSpan callback now lands on the fork and no longer persists after it. That was the intended Sentry semantic all along, but it is a behavior change. Note anyone pinned ~2.1.0 gets no PII-leak fix at all under a minor.
  • console.error vs throwing in the guard, given sendDefaultPii: true is set. Log-and-continue means a detected isolation failure still serves traffic.

Not in this PR

Real, none required for AI-3346. Detail in .context/todos.md.

  1. buildTracesSampler throws on array attribute values. rawRoute.replace on an http.target of ['/a'] gives TypeError, from inside SentrySampler.shouldSample, on every span. Found while probing the flush path.
  2. The resource does not merge defaultResource(), so OTEL_RESOURCE_ATTRIBUTES and all telemetry.sdk.* are missing from exported spans.
  3. resetForTesting() does not clear the OTel globals, so a documented reset-then-init() is silently refused, and the guard is blind to it. The new test hand-rolls clearOtelGlobals() for this reason.
  4. client.asyncLocalStorageLookup never assigned (latent; nothing in @sentry 10.34 reads it).
  5. Only the context-manager registration is checked; setGlobalTracerProvider / setGlobalPropagator fail just as silently and their booleans are discarded.

Before the agentic-api bump

Audit every withIsolationScope call site first. Those wraps are no-ops today; once this ships, tags set inside one stop being visible outside it. Any capture currently reading a tag it never set (and getting the right answer via the leak) starts reading nothing. PR #6738 there was written on the false premise that adding wraps would help, when the wrap was the broken thing.

Sentry issue grouping will also shift, since transaction is wrong on many events today, so expect a wave of apparently-new issues that are regrouped existing ones. Announce before it lands.

🤖 Generated with Claude Code

`initializeSentry` passes `skipOpenTelemetrySetup: config.enableOtel`, which
skips `initOpenTelemetry()`. That is the only place @sentry/node installs
`SentryContextManager`, while @sentry/node-core's `init` installs the
OpenTelemetry async context strategy unconditionally. Sentry's scope functions
were therefore backed by the OTel context with nothing writing scopes onto it,
so `withIsolationScope` and `getCurrentScope` resolved to the process-global
scope and tags, user identity and transaction names crossed between concurrent
requests and background jobs.

`provider.register()` now installs `SentryContextManager`.

The same skipped setup also leaves `client.traceProvider` unassigned, which is
the only handle `NodeClient.flush()` and `.close()` have on the span processors,
so a graceful shutdown discarded whatever the OTLP batch was holding.
`attachTraceProviderForShutdownFlush` supplies it, bounded by
`forceFlushTimeoutMillis` and with span export failures contained, because
`BasicTracerProvider.forceFlush()` rejects with an array of errors and
`NodeClient.flush()` awaits it uncaught -- an unreachable collector would
otherwise abort the client's own event flush and drop buffered errors.
It is skipped when the client already has a provider, so a consumer that
completed its own OpenTelemetry setup keeps the one that actually receives spans.

`warnIfScopeIsolationInactive` reports when isolation is inactive anyway.
`setGlobalContextManager` refuses to replace an existing manager and reports the
refusal only on OpenTelemetry's diag channel; neither SDK validator catches that,
since `validateOpenTelemetrySetup` early-returns in non-debug builds and
`openTelemetrySetupCheck` records elements at construction rather than at
successful registration.

Refs AI-3346

Co-Authored-By: Claude <noreply@anthropic.com>
@andybevan-scope3
andybevan-scope3 marked this pull request as ready for review July 29, 2026 21:53

@benminer benminer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the isolation and shutdown changes. The OTLP shutdown path is now bounded and covered by a close-path regression test.

@benminer
benminer merged commit 885d537 into main Jul 29, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants