fix(node): restore tsx runtime instrumentation by jpdefo · Pull Request #25351 · getsentry/sentry-javascript

jpdefo · 2026-10-14T07:52:34Z

Restore runtime instrumentation for CommonJS dependencies loaded through tsx on Node 24.13+. Keep synchronous module hooks and use the CommonJS compiler for custom require loaders. Match canonical file URLs and source fingerprints to avoid transforming the same source twice while preserving the fallback for source changed by another loader.

Regression coverage checks native Node and tsx with CommonJS reloads, query and fragment imports, and cache eviction after queried imports. The tsx-express E2E application now inherits Node 24 and all 10 existing tests pass locally. The two existing nft tracing tests fail on Windows with both this patch and the original code.

Fixes #24787

Fixes getsentry#24787. Signed-off-by: João Pedro de Faria <jpdefo7@hotmail.com> Co-Authored-By: GPT-6.1 Sol <codex@openai.com>

JPeer264 · 2026-10-14T07:58:35Z

Thanks a lot for opening the PR. Before I go deeper on the review could you remove this line to run this specific e2e test again on Node v24?

sentry-javascript/dev-packages/e2e-tests/test-applications/tsx-express/package.json

Line 33 in f47c08b

"node": "20.19.5",

cursor

Stale Bugbot comment from a previous run.

Run the tsx-express E2E application on the inherited Node 24 runtime. Co-Authored-By: GPT-6.1 Sol <codex@openai.com>

jpdefo · 2026-10-14T09:18:49Z

Removed the tsx-express application's Node 20 override so it inherits Node 24.21.0. All 10 existing tsx-express E2E tests pass on that version with the updated local SDK packages, without changing assertions or timeouts.

Also addressed the URL-key issue reported by Bugbot. The duplicate-transform guard now normalizes query/fragment URLs and compares source fingerprints before skipping the compiler transform, preserving the fallback when another loader changes the source. Added query, fragment, and cache-eviction coverage. All eight focused regressions, the full development build, and lint pass. The runtime-injection unit suite has 18 passes and the same two previously reproduced Windows nft tracing failures.

JPeer264 · 2026-10-14T14:27:48Z

bugbot run

cursor

Stale Bugbot comment from a previous run.

Under Node >= 24.13 the SDK injects instrumentation via Module.registerHooks, which tsx's CJS loader bypasses. Keep the pin until #25351 lands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Most e2e apps that pinned their own Node version did so to run on something newer than the old Node 20 base. Since #24343 moved the root to Node 24.21.0, those pins are either redundant or now *older* than the root (e.g. the `24.15.0` pins), so every app now simply extends the root Volta config. This also drops the Node 20 pin that #24343 added to `react-send-to-sentry` as a stopgap, since it passes on Node 24 now. Two overrides remain: - pnpm `11.10.0` in `nuxt-5` and `nuxt-5-sourcemaps`, which was bumped deliberately in #22017. - Node `20.19.5` in `tsx-express`. On Node >= 24.13 the SDK injects instrumentation through `Module.registerHooks`, whose `load` hook tsx's CJS loader bypasses (it replaces `Module._extensions` and calls `_compile` directly), so Express is never instrumented. #25351 fixes this; the pin can go once it lands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

JPeer264

I think it works overall. I added some comments and once they're addressed we can merge this.

JPeer264 · 2026-10-15T14:21:24Z

    if (typeof mod.registerHooks === 'function' && stableSyncHooks) {
      initialize({ instrumentations: SENTRY_RUNTIME_INSTRUMENTATIONS });
-      mod.registerHooks({ resolve, load: globalAny.Deno ? withDenoFormats(load) : load });
+      const isNode = !globalAny.Bun && !globalAny.Deno;


Is there a specific reason why you exclude Bun and Deno for this? Based on my research it should actually work in Deno and Bun wouldn't actually hit this line as the typeof mod.registerHooks === 'function' would prevent that.

So I guess removing the isNode check would work OOTB already.

FWIW our node-integration-tests automatically run on Bun and Deno, so we would check against these too.

You’re right, the additional isNode guard was unnecessary. I’ve removed it and updated the unit test to verify that Deno installs the CommonJS fallback. The targeted Deno integration tests pass, and I confirmed that Bun 1.4.0 does not expose registerHooks, so the existing capability check handles it.

JPeer264 · 2026-10-15T14:27:48Z

+
+const DataLoader = require('dataloader');
+const value = await new DataLoader(async keys => keys).load('real-key');
+await Sentry.close(1000);


Is it specifically required to close the client? I think we could remove the line to check it works without it

I removed Sentry.close(1000) and ran the reload test with both plain Node and tsx. Both exited normally and recorded exactly one operation and one injection. The same scenario also passed directly on Deno.

Allow the CommonJS fallback on runtimes with stable synchronous hooks and exercise reloads without closing the client. Co-Authored-By: GPT-6.1 Sol <codex@openai.com>

jpdefo · 2026-10-16T09:42:38Z

@JPeer264 It's probably ready now, please double check the changes. Thank you!

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 1efa955. Configure here.}

cursor · 2026-10-16T09:48:27Z

  // `Module.registerHooks` / `Module.register` are newer than the @types/node
  // we build against, hence the cast.
-  const mod = Module as NodeModule;
+  const mod = Module as unknown as NodeModule;


Double-cast lacks typing justification

Low Severity

The new as unknown as NodeModule double-cast has no comment explaining why a safer type is impossible. The nearby note only covers registerHooks / register, not the added Module.prototype._compile shape that forced the double-cast. Flagged because the PR review rules require that justification on new SDK double-casts.

^{Triggered by project rule: PR Review Guidelines for Cursor Bot}

^{Reviewed by Cursor Bugbot for commit 1efa955. Configure here.}

fix(node): restore tsx runtime instrumentation

9d5002b

Fixes getsentry#24787. Signed-off-by: João Pedro de Faria <jpdefo7@hotmail.com> Co-Authored-By: GPT-6.1 Sol <codex@openai.com>

github-actions Bot added the external PR from an external contributor label Oct 14, 2026

jpdefo marked this pull request as ready for review October 14, 2026 07:55

jpdefo requested a review from a team as a code owner October 14, 2026 07:55

jpdefo requested review from isaacs and mydea and removed request for a team October 14, 2026 07:55

JPeer264 self-requested a review October 14, 2026 07:57

cursor Bot reviewed Oct 14, 2026

View reviewed changes

Comment thread packages/server-runtime-injection/src/register.ts Outdated

fix(node): avoid repeated instrumentation for queried modules

a255fd3

Run the tsx-express E2E application on the inherited Node 24 runtime. Co-Authored-By: GPT-6.1 Sol <codex@openai.com>

cursor Bot reviewed Oct 14, 2026

View reviewed changes

mydea mentioned this pull request Oct 14, 2026

test(e2e): Inherit root Volta config in all e2e test apps #25369

Merged

JPeer264 reviewed Oct 15, 2026

View reviewed changes

jpdefo and others added 2 commits October 16, 2026 06:38

fix(node): apply runtime instrumentation review feedback

9a97d9c

Allow the CommonJS fallback on runtimes with stable synchronous hooks and exercise reloads without closing the client. Co-Authored-By: GPT-6.1 Sol <codex@openai.com>

Merge branch 'develop' into feat/tsx-runtime-injection

1efa955

cursor Bot reviewed Oct 16, 2026

View reviewed changes

Conversation

jpdefo commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JPeer264 commented Oct 14, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpdefo commented Oct 14, 2026

Uh oh!

JPeer264 commented Oct 14, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JPeer264 left a comment

Choose a reason for hiding this comment

Uh oh!

JPeer264 Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

jpdefo Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

JPeer264 Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

jpdefo Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

jpdefo commented Oct 16, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Oct 16, 2026

Choose a reason for hiding this comment

Double-cast lacks typing justification

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jpdefo commented Oct 14, 2026 •

edited

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading