fix(test): eliminate vitest isolate:false cross-file state leaks#5964
Conversation
setSnapshotModuleLoader() set a module-level loader and flipped the module-level `isESM` to false with no way to unset it, and snapshot-import.test.ts had a no-op afterEach. Under vitest `isolate: false` (root config uses pool:threads + isolate:false) the module registry is shared across files, so after the snapshot test ran every later file resolved modules in CJS + snapshot mode and failed with `Can not find plugin @eggjs/<x>` / `Cannot find module '@eggjs/<x>/package.json'`. Make setSnapshotModuleLoader(undefined) clear the loader and restore the auto-detected isESM, and clear it in the test teardown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mockContext() reused this.currentContext without checking the context belongs to this app. When the async-local context storage is shared across app instances (multiple mm.app() apps in one realm, or vitest `isolate: false` carry-over) app2.mockContext() could return app1's context, binding helpers/services to the wrong app config — e.g. security `surl` custom-protocol returning '' and csrf 401s. Guard the reuse with `this.currentContext.app === this`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFixes two cross-realm test-state leaks under Vitest with ChangesVitest isolate:false State Leak Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying egg-v3 with
|
| Latest commit: |
d8c6128
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://adda386e.egg-v3.pages.dev |
| Branch Preview URL: | https://worktree-vitest-isolate-fals.egg-v3.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses nondeterministic test failures caused by cross-file state leaks when running Vitest with isolate: false. It fixes module-resolution poisoning in @eggjs/utils by restoring the auto-detected isESM state when clearing the snapshot module loader, and ensures that @eggjs/mock only reuses active async-local contexts belonging to the current application instance. Additionally, it documents these isolation issues and their fixes in the workspace wiki. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR fixes two confirmed realm-global state leaks that show up when the monorepo test suite runs under Vitest with pool: 'threads' + isolate: false, where multiple test files share the same Node realm (module registry / globalThis / module-level state) and can nondeterministically affect each other.
Changes:
- Add an explicit “unset” path for
setSnapshotModuleLoader()that clears the snapshot loader and restores the originally auto-detectedisESMmode. - Ensure the snapshot-import test suite resets snapshot-loader state in
afterEach()to prevent cross-file contamination. - Prevent
@eggjs/mockmockContext()from reusing an async-local context that belongs to a different app instance.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/utils/src/import.ts |
Makes snapshot-loader state reversible by restoring isESM when clearing the loader. |
packages/utils/test/snapshot-import.test.ts |
Adds teardown to clear snapshot-loader state after each test. |
plugins/mock/src/app/extend/application.ts |
Guards context reuse to avoid returning a context from a different app instance. |
wiki/concepts/vitest-isolate-false-state-leaks.md |
Adds a concept doc explaining isolate:false leak mechanics and the specific fixes. |
wiki/index.md |
Links the new concept doc from the wiki index. |
wiki/log.md |
Logs the diagnosis and the source files/pages updated for this work. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5964 +/- ##
==========================================
+ Coverage 85.30% 85.32% +0.02%
==========================================
Files 670 670
Lines 19552 19553 +1
Branches 3863 3864 +1
==========================================
+ Hits 16678 16683 +5
+ Misses 2481 2479 -2
+ Partials 393 391 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
d8c6128
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://363afc32.egg-cci.pages.dev |
| Branch Preview URL: | https://worktree-vitest-isolate-fals.egg-cci.pages.dev |
Record the root causes (import.ts snapshot loader / isESM leak, mock mockContext wrong-app reuse), the CI-faithful triage method (Node 22/24 + utoo install, run-alone rule), and which remaining failures are environmental or pre-existing load flakes rather than isolate bugs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
worktree-vitest-isolate-false
branch
from
47b0ce5 to
d8c6128
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wiki/concepts/vitest-isolate-false-state-leaks.md (1)
24-29: ⚡ Quick winAdd explicit
Inference:/ staleness qualifiers for synthesized and time-sensitive claims.The nondeterminism diagnosis and “15 → 3 failing files” outcome are synthesis and point-in-time observations, but they are not explicitly marked. Please tag those statements with
Inference:and add a freshness qualifier (for example, “as of 2026-06-08”) to align with wiki policy.As per coding guidelines: “List major source paths in
source_filesfrontmatter and mark non-obvious synthesis asInference:” and “mark stale or unresolved claims when freshness is uncertain.”Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/concepts/vitest-isolate-false-state-leaks.md` around lines 24 - 29, Mark the synthesized, time-sensitive statements by prepending "Inference:" and adding a freshness qualifier (e.g., "as of 2026-06-08") to the nondeterminism diagnosis and the outcome phrase; specifically update the sentence containing "failures are **nondeterministic**" and the phrase "15 → 3 failing files" to read with an "Inference:" prefix and a date qualifier, and apply the same treatment to the similar claims around lines referenced as 91-93 so all non-obvious synthesis and potentially stale observations are explicitly labeled and dated.Source: Coding guidelines
wiki/log.md (1)
9-9: ⚡ Quick winMark synthesized diagnosis text as
Inference:in the log note.This note mixes direct facts with causal synthesis (why failures happened and why remaining ones are excluded). Prefix the synthesized portion with
Inference:so the log conforms to the wiki evidence-labeling rule.As per coding guidelines: “mark non-obvious synthesis as
Inference:”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/log.md` at line 9, The log note mixes observed facts with synthesized causal explanation; update the entry so the synthesized diagnosis is prefixed with "Inference:" — for example, in the sentence describing the two realm-global leaks, prepend "Inference:" before the explanation that (1) setSnapshotModuleLoader left `_snapshotModuleLoader`/`isESM=false` set and (2) mock.mockContext() reused `currentContext`, so the factual repro details remain unchanged but the causal synthesis is clearly labeled as inference.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wiki/concepts/vitest-isolate-false-state-leaks.md`:
- Around line 24-29: Mark the synthesized, time-sensitive statements by
prepending "Inference:" and adding a freshness qualifier (e.g., "as of
2026-06-08") to the nondeterminism diagnosis and the outcome phrase;
specifically update the sentence containing "failures are **nondeterministic**"
and the phrase "15 → 3 failing files" to read with an "Inference:" prefix and a
date qualifier, and apply the same treatment to the similar claims around lines
referenced as 91-93 so all non-obvious synthesis and potentially stale
observations are explicitly labeled and dated.
In `@wiki/log.md`:
- Line 9: The log note mixes observed facts with synthesized causal explanation;
update the entry so the synthesized diagnosis is prefixed with "Inference:" —
for example, in the sentence describing the two realm-global leaks, prepend
"Inference:" before the explanation that (1) setSnapshotModuleLoader left
`_snapshotModuleLoader`/`isESM=false` set and (2) mock.mockContext() reused
`currentContext`, so the factual repro details remain unchanged but the causal
synthesis is clearly labeled as inference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf60ed5a-d4c3-4d10-8a01-db51e1fc4c40
📒 Files selected for processing (3)
wiki/concepts/vitest-isolate-false-state-leaks.mdwiki/index.mdwiki/log.md
✅ Files skipped from review due to trivial changes (1)
- wiki/index.md
Motivation
The root
vitest.config.tsruns the whole monorepo withpool: 'threads'+isolate: false. Under this mode every test file in a worker shares one Node realm (module registry,globalThis, module-level state). A couple of modules cached realm-global state without resetting it, which leaked across files/projects and produced nondeterministic CI failures (different files fail run-to-run).This PR fixes the two deterministic leaks at their source.
Root causes & fixes
@eggjs/utilsimport.ts— snapshot loader /isESMpoisoning (cross-project).setSnapshotModuleLoader()set a module-level loader and flipped module-levelisESMtofalsewith no way to unset, andsnapshot-import.test.ts'safterEachwas a no-op. After that test ran, every later file in the worker resolved modules in CJS + snapshot mode and failed withCan not find plugin @eggjs/<x>/Cannot find module '@eggjs/<x>/package.json'(ajv-plugin, typebox-validate, view-nunjucks, standalone, …).setSnapshotModuleLoader(undefined)now clears the loader and restores the auto-detectedisESM; the test clears it inafterEach.@eggjs/mockmockContext()— wrong-app context reuse.mockContext()reusedthis.currentContextwithout checking the context belongs tothisapp. With a shared/lingering async-local context (multiplemm.app()apps in one realm, orisolate:falsecarry-over),app2.mockContext()returned app1's context, binding helpers/services to the wrong app config — e.g.security/surlcustom-protocol returning'',security/csrf401s.this.currentContext.app === this.Test evidence (local, Node 22, utoo install)
isolate:false,--retry 0: 15 failing files → 3.orm-plugin(needs MySQL),security/ssrf(needs DNS) — both pass in CI;multipart/file-modeis a pre-existing load flake that also fails underisolate:trueand is absorbed by CI's--retry 2.--retry 2, the only remaining local failures are the two environmental ones above.See
wiki/concepts/vitest-isolate-false-state-leaks.mdfor the full diagnosis and triage method.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
pool: 'threads'+isolate: false, plus a dated log entry.