Skip to content

test(node-integration-tests): Enable Node compile cache for scenario child processes#22084

Open
mydea wants to merge 1 commit into
developfrom
feat/node-it-compile-cache
Open

test(node-integration-tests): Enable Node compile cache for scenario child processes#22084
mydea wants to merge 1 commit into
developfrom
feat/node-it-compile-cache

Conversation

@mydea

@mydea mydea commented Jul 8, 2026

Copy link
Copy Markdown
Member

Every createRunner scenario spawns a fresh node child process that imports the full @sentry/node + OpenTelemetry module graph from scratch — which is by far the dominant per-test cost (~200-300ms). This points all child processes at one shared NODE_COMPILE_CACHE directory so V8 caches the parse/compile of that graph and reuses it across the thousands of child processes a test run spawns.

The env var is silently ignored on Node < 22, so we gate on the parent process major version (the same node binary the children run) to avoid creating an unused cache dir there. A user-set NODE_COMPILE_CACHE in the environment still takes precedence.

Measured (Node 24): ~14% faster CJS child startup; ESM sees little change since import-in-the-middle dominates its loader path. No effect on Node 20 (env var ignored).

🤖 Generated with Claude Code

…child processes

Point every scenario child process at a shared `NODE_COMPILE_CACHE` directory
so the V8 parse/compile of the `@sentry/node` + OpenTelemetry module graph —
loaded from scratch by each spawned process — is cached and reused across runs.

The env var is silently ignored on Node < 22, so we only set it when the parent
(the same `node` binary the children run) is Node 22+, and a user-set
`NODE_COMPILE_CACHE` still takes precedence.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mydea mydea requested a review from a team as a code owner July 8, 2026 12:02
@mydea mydea requested review from JPeer264 and andreiborza and removed request for a team July 8, 2026 12:02
Comment on lines +422 to +424
? {
...COMPILE_CACHE_ENV,
...process.env,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The code sets the NODE_COMPILE_CACHE variable but never creates the cache directory, causing Node.js to silently disable caching.
Severity: MEDIUM

Suggested Fix

Ensure the directory specified in the NODE_COMPILE_CACHE environment variable is created before any child processes that would use it are spawned. Use a method like fs.mkdirSync(cachePath, { recursive: true }) to create the directory during the runner's initialization phase.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: dev-packages/node-integration-tests/utils/runner/createRunner.ts#L422-L424

Potential issue: The code sets the `NODE_COMPILE_CACHE` environment variable to a path
for caching compiled code, but it never creates the specified directory. According to
Node.js documentation, if the directory pointed to by `NODE_COMPILE_CACHE` does not
exist when the Node process starts, caching is silently disabled. This means the
intended performance optimization will not take effect, and the feature will fail
silently without any errors or warnings. The expected performance improvement from this
change will not be realized.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this should be auto-created by node

@mydea

mydea commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Does not appear much faster on CI but always hard to benchmark these things...

@andreiborza andreiborza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hard to tell if this can lead to issues but looks good I guess 😅

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