test: add regression tests proving concurrent trace() calls corrupt Sentry async-context state - #45249
test: add regression tests proving concurrent trace() calls corrupt Sentry async-context state#45249MajorLift wants to merge 3 commits into
Conversation
`shared/lib/trace.ts`'s `startSpan()` wraps every span in
`sentryWithIsolationScope()`, which relies on `@sentry/browser`'s
stack-based async-context strategy -- a single, shared, mutable stack per
JS realm, since no `AsyncLocalStorage`/Zone equivalent exists in a
browser/service-worker environment. These tests exercise the real SDK
end-to-end (a real `BrowserClient`, not the file's default mocked
`globalThis.sentry`) to force two `trace()` calls to interleave
deterministically, and show that a concurrently-pending, logically
unrelated call gets silently parented under -- and adopts the trace id of
-- whichever operation's layer is currently on top of the shared stack.
`sentry-trace-propagation.concurrency.test.ts` covers the same root cause's
second symptom: `getCurrentTraceId()` correlating an outbound
`consensys-request-id` with the wrong concurrently-running operation's
trace id. It's a separate file rather than a new block in
`sentry-trace-propagation.test.ts` because that file's file-wide
`jest.mock('@sentry/browser')` / `jest.mock('@sentry/core')` is
incompatible with exercising the real SDK the defect actually lives in.
No fix exists yet, so the corruption-revealing assertions are marked
`it.failing` -- CI stays green, and flipping them back to `it` once a fix
lands will fail loudly if the fix is incomplete. See
MetaMask-planning#7523.
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [62dfa62] [reused from 4a98074]
⚡ Performance Benchmarks (Total: 🟢 10 pass · 🟡 7 warn · 🔴 4 fail)
Bundle size diffs
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
`it.failing` type-checks fine in isolation, but this repo also has `@types/mocha` installed, which declares a conflicting global `it` with no `.failing` member -- `yarn lint:tsc` picked the wrong merged type and failed with `Property 'failing' does not exist on type 'TestFunction'`. Switched both new-bug-revealing tests to `it.skip` instead, which is what this repo's own tooling can actually support; `jest/no-disabled-tests` is `error`-level for `shared/**/*.test.ts` (not configured at all for `app/scripts/lib/**/*.test.ts`, a separate pre-existing gap in `.eslintrc.js`'s file globs), so `shared/lib/trace.test.ts`'s two skips get an explicit, comment-justified `eslint-disable-next-line`. Also fixes: - `Sentry.Client` isn't exported from `@sentry/browser`'s type namespace; import `Client` from `@sentry/core` instead, matching the existing `sentry-trace-propagation.ts`. - `trace()`'s public `TraceCallback<T>` type is `(context?: TraceContext) => T`, not `(span: Sentry.Span | null) => T` -- reading the callback's own parameter for the real Sentry span (as the original version of these tests did) doesn't type-check. Read `Sentry.getActiveSpan()` from inside the callback instead, which also more faithfully mirrors how production code (`getSerializedTraceContext()`) actually reads it. - Generic type parameter `T` was too short for `@typescript-eslint/naming-convention`'s minimum-length rule; renamed to `Value`. - `_traceId`/`_spanId`/`trace_id` object-literal keys (required by `hasDistributedTraceIds`'s and Sentry's own shapes) now carry the `eslint-disable-next-line @typescript-eslint/naming-convention` comment already used elsewhere in this codebase for the same shapes. Verified after each fix: `yarn jest` green (3 skipped, 43 passed), the three skipped tests still fail for the intended reason when temporarily un-skipped, `yarn lint:eslint` clean on both files, `yarn lint:tsc` clean project-wide.
Builds ready [3cd1d07] [reused from 4a98074]
⚡ Performance Benchmarks (Total: 🟢 10 pass · 🟡 7 warn · 🔴 4 fail)
Bundle size diffs
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3cd1d07. Configure here.
| // (candidate directions are in the ticket), remove `.skip` -- if it's | ||
| // still red at that point, the fix is incomplete. | ||
| // eslint-disable-next-line jest/no-disabled-tests | ||
| it.skip('does not parent the second span under the still-pending first one', async () => { |
There was a problem hiding this comment.
Skipped tests lose fix detection
Medium Severity
These bug-reproducing cases were switched from it.failing to it.skip, so they never run. That drops the PR’s stated guard: today the defect stays invisible in CI, and a later fix won’t surface as an unexpected pass. The new comments also imply the alternative was a red suite, which it.failing already avoids.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 3cd1d07. Configure here.
MajorLift
changed the title
CI's `Test lint` job runs `yarn lint`, which includes `yarn lint:format`
(`oxfmt -c oxfmt.config.mts --check`) ahead of ESLint and tsc -- a check
this repo's own coding-guidelines skill calls out separately from ESLint
("Do not run Prettier directly on code files; Prettier remains for JSON
formatting"), and one this PR's earlier commits never ran. No behavioral
change; `yarn jest` and `yarn lint:eslint` both still pass unchanged.
Builds ready [a11a6b8] [reused from 4a98074]
⚡ Performance Benchmarks (Total: 🟢 10 pass · 🟡 7 warn · 🔴 4 fail)
Bundle size diffs
|
|
🧪 Validation Run
Verdict: ✅ proven — Claim: concurrent Forced-interleaving harness against the real, installed Guarantee table
The three Live-telemetry corroboration — rows 2–3 (parent-span misattribution). Both specimens below meet the same bar: a parent-child edge that the parent's own implementation, read at a pinned commit, has no code path to produce. Specimen —
Specimen — production ( That trace's full op breakdown sums to 453 spans over 18.7 seconds — small and short next to the hours-long mega-trace pattern noted further down. 327 of the 349 A third cluster matched the shape but is not a defect. This trace shows
Implementation — forced-interleaving unit test suite (regression coverage)Interleaving-forcing check. Every forced-interleaving test in Mutation check ( Exhibit — jest run (this PR's own two new test files, plus the sibling file they extend — Same result, from GitHub's own CI (not a local run) — Unit tests (3) and Unit tests (5), the two shards that drew these files, head Gist is a reproducibility backstop for the exact jest invocation above — a re-run path, not a substitute for the block already shown. Follows from the above
Fix direction — decided in MetaMask-planning#7523, not implemented in this PR. Explicit context propagation, which is what Sentry's own engineers recommend for this defect class on getsentry/sentry-javascript#4071: "explicit propagation of hubs and scopes will always work — the issues arise when you try to implicitly propagate it. In the case of implicit propagation, the auto-instrumentation has to 'pick' the correct hub/scope to call methods on, which breaks down in async environments." Concretely: pass Open for review — one branch is not covered here. |
1 participant
Summary
Adds regression tests that prove — via forced deterministic interleaving against the real
@sentry/browserSDK, not mocks — the concurrency defect tracked in MetaMask-planning#7523: concurrenttrace()calls in the service worker can corrupt each other's Sentry async-context state.No fix is included. This PR's purpose is to make the confirmed-but-unfixed bug visible and provable in the codebase (a red assertion documented and gated, not a silent gap), per MetaMask-planning#7523's acceptance criteria:
trace()calls. Reproduced.getCurrentTraceId()correlating aconsensys-request-idwith the wrong concurrently-running operation's trace id. Reproduced, for the realistic concurrent-RPC scenario.The claim being tested
shared/lib/trace.ts'sstartSpan()wraps every span insentryWithIsolationScope().@sentry/browserregisters no alternative async-context strategy for browser/service-worker environments (noAsyncLocalStorage/Zone equivalent — only@sentry/server-utilsand@sentry/opentelemetryregister one), so it falls back to a stack-based implementation:AsyncContextStack(node_modules/@sentry/core/.../asyncContext/stackStrategy.js), a single, shared, mutable array for the whole JS realm's lifetime.When two
trace()calls overlap — ordinary event-loop behavior for concurrent RPC handlers,wrapMessengerWithTracing-wrapped calls, or websocket notification handling in the service worker — a later call's layer can still be on top of that shared stack when an earlier, still-pending call's own continuation resumes and reads "the current active span." The read is misattributed to the wrong logical operation. This isn't a rare edge case; it's what the shared stack does on every overlap by construction.Mechanism correction from the original ticket
The ticket's original write-up described this as isolation-scope cross-contamination ("B's layer pushes on top of A's isolation scope"). Reading the actual SDK source (
@sentry/corev10.38.0, this repo's installed version) turned up a more specific mechanism:AsyncContextStack._isolationScope) for the whole realm's lifetime. The SDK's own docstring confirms this: "If no async context strategy is set, the isolation scope and the current scope will not be forked (this is currently the case, for example, in the browser)."_stack).Sentry.startSpan()pushes a second layer onto that same shared array, andshared/lib/trace.ts:538-544's active-span-inheritance code (sentryGetActiveSpan(), used when no explicitparentContextis given) reads whatever's on top of it at call time.Mutation-check implication for any future fix
Disabling just
trace.ts's active-span-inheritance shortcut (lines 538-544) does not fix the defect — confirmed empirically in this PR's own harness.Sentry.startSpan()'s own current-scope cloning independently inherits the ambient trace id from whatever is on top of the shared stack, regardless of that one shortcut. The same holds forgetCurrentTraceId()'sgetActiveSpan()branch: disabling it doesn't flip the correlation defect, because the fallback path (getCurrentScope().getPropagationContext()) reads from the exact same corrupted shared stack-top and independently carries the same wrong trace id.Any real fix needs to address the shared, unforked current-scope stack itself — not just patch one of the two call sites that happen to read it. Candidate directions are listed in MetaMask-planning#7523; this PR takes no position on which one.
What's in this PR
shared/lib/trace.test.ts— newdescribe('concurrent trace() calls (MetaMask-planning#7523)', ...)block. Exercises the real@sentry/browserSDK (a realBrowserClient,globalThis.sentrywired to the real SDK) instead of this file's default mock, because the defect lives inside the SDK's realAsyncContextStack.app/scripts/lib/sentry-trace-propagation.concurrency.test.ts— new file, not a new block in the existingsentry-trace-propagation.test.ts. That file does a file-widejest.mock('@sentry/browser')/jest.mock('@sentry/core'), which is incompatible with exercising the real SDK end-to-end (the module under test binds itsgetActiveSpan/getCurrentScope/getIsolationScopeimports at its own first load, so partially un-mocking within that file wouldn't change what the module itself sees).Both files force the interleaving deterministically with hand-controlled deferred
Promises and microtask stepping — no timing-dependent flakiness, no real timers.Why the bug-revealing tests are
it.skip, not left redThere is no fix yet, so the assertions that state the correct (post-fix) behavior currently fail. Rather than delete them or leave the suite red with no explanation, three tests are
it.skip, each with a comment stating: what's currently wrong, exactly what removing.skipshows today, and a pointer to MetaMask-planning#7523.it.failing(Jest 29's built-in "expected failure" mechanism, which would let these run and self-document without a human needing to remove.skip) was tried first and reverted: it type-checks fine in isolation, but this repo also has@types/mochainstalled, which declares a conflicting globalitwith no.failingmember —yarn lint:tscresolves the wrong merged type project-wide and fails withProperty 'failing' does not exist on type 'TestFunction'.it.skipis what this repo's actual toolchain supports for this.shared/lib/trace.test.ts's two skips carry an expliciteslint-disable-next-line jest/no-disabled-tests(that rule iserror-level forshared/**/*.test.ts);sentry-trace-propagation.concurrency.test.ts's skip needs no such comment because.eslintrc.js's jest-rules file list doesn't currently coverapp/scripts/lib/**/*.test.tsat all (a separate, pre-existing gap called out in that file's ownTODO, not something this PR relies on or should be read as endorsing).Each skipped test is paired with a sequential/discriminating control (a structurally identical test with the interleaving removed) that currently passes — proving the harness discriminates real overlap from no-overlap, and isn't just failing for an unrelated reason. All three skipped tests were verified, by temporarily removing
.skip, to fail for the intended reason (not a vacuous or incidental failure) before being re-skipped for this PR.Related
Test plan
yarn jest shared/lib/trace.test.ts app/scripts/lib/sentry-trace-propagation.concurrency.test.ts app/scripts/lib/sentry-trace-propagation.test.ts— all green (3 skipped, 43 passed).skipand inspecting the actual thrown error, then re-skippedyarn lint:eslintclean on both changed filesyarn lint:tscrun project-wide: zero errors in either of this PR's two files. (The full run exits non-zero on pre-existing, unrelated errors elsewhere in the tree — e.g.ui/hooks/ramps/utils/mapRampsOrderSafely.ts,app/scripts/messenger-client-init/**— present onmainbefore this PR; confirmed by file path, not introduced here.)yarn lint:format(oxfmt) clean on both changed filesCHANGELOG entry: null
Note
Low Risk
Changes are limited to new and extended test files with no production or runtime behavior changes.
Overview
Adds test-only regression coverage for MetaMask-planning#7523: overlapping
trace()calls against the real@sentry/browserSDK can corrupt shared async-context state in the service worker (no fix in this PR).shared/lib/trace.test.tsgains aconcurrent trace() callssuite that uses a realBrowserClientinstead of the file’s mockedglobalThis.sentry. It asserts span parenting stays independent when operations overlap; twoit.skipcases document the current wrong behavior (child spans parented under a still-pending unrelated trace, including a third concurrent call). A sequential case passes as a baseline.app/scripts/lib/sentry-trace-propagation.concurrency.test.tsis a new file (separate from the fully mockedsentry-trace-propagation.test.ts) that wiresconsensysTracePropagationIntegrationto real fetch instrumentation and checksconsensys-request-id/ baggage correlation under forced interleaving with distinct distributed trace ids. One skipped test encodes the bug (A’s fetch correlated with B’s trace); a control test passes when B finishes before A resumes.Skipped tests are intentional so CI stays green until a fix lands; each skip is paired with a passing control that isolates overlap as the failure driver.
Reviewed by Cursor Bugbot for commit a11a6b8. Bugbot is set up for automated code reviews on this repo. Configure here.