fix(loader-fs): ignore vitest environment-teardown import races#5980
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesVitest Teardown Error Handling in loadFile
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 docstrings
🧪 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 with
|
| Latest commit: |
ecd2b4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://72b774fd.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-mock-teardown-import-rac.egg-cci.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 introduces a mechanism to detect and gracefully handle Vitest's EnvironmentTeardownError during dynamic imports in RealLoaderFS.loadFile. By swallowing this specific error and returning undefined, it prevents stray dynamic imports from causing unhandled rejections after a test environment has been torn down. The review feedback suggests improving the type safety and robustness of the error-traversing helper isEnvironmentTeardownError by explicitly checking that the traversed value is an object or function before accessing its properties.
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.
| function isEnvironmentTeardownError(e: unknown): boolean { | ||
| let cur = e as { name?: unknown; message?: unknown; cause?: unknown } | undefined | null; | ||
| for (let depth = 0; cur && depth < 5; depth++) { | ||
| if (cur.name === 'EnvironmentTeardownError') return true; | ||
| if (typeof cur.message === 'string' && cur.message.includes('after the environment was torn down')) { | ||
| return true; | ||
| } | ||
| cur = cur.cause as typeof cur; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
To improve type safety and runtime robustness, we should explicitly check that cur is an object or function before accessing its properties. Although JavaScript allows property access on most primitives, explicitly verifying the type prevents potential issues and aligns with cleaner TypeScript practices.
function isEnvironmentTeardownError(e: unknown): boolean {
let cur = e;
for (let depth = 0; cur && depth < 5; depth++) {
if (typeof cur === 'object' || typeof cur === 'function') {
const err = cur as { name?: unknown; message?: unknown; cause?: unknown };
if (err.name === 'EnvironmentTeardownError') return true;
if (typeof err.message === 'string' && err.message.includes('after the environment was torn down')) {
return true;
}
cur = err.cause;
} else {
break;
}
}
return false;
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5980 +/- ##
=======================================
Coverage 85.59% 85.60%
=======================================
Files 669 669
Lines 19892 19904 +12
Branches 3942 3947 +5
=======================================
+ Hits 17026 17038 +12
Misses 2478 2478
Partials 388 388 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
ecd2b4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://09c463a3.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-mock-teardown-import-rac.egg-v3.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/loader-fs/test/fixtures/loadfile/teardown-error.js (1)
1-10: 💤 Low valueConsider using
.tsor.mjsextension for test fixtures.The coding guidelines specify using TypeScript throughout all packages. While this fixture serves its purpose as
.js, consider renaming it to.tsor.mjsfor consistency with the monorepo's TypeScript-first approach.♻️ Optional refactor to TypeScript
Rename to
teardown-error.ts:-'use strict'; - // Simulates a module whose import loses the race with a vitest test-environment // teardown: the runtime raises an `EnvironmentTeardownError`. `RealLoaderFS.loadFile` // should treat this as a benign no-op and resolve `undefined` instead of throwing. const err = new Error( "Cannot load 'teardown-error.js' imported from import.ts after the environment was torn down. This is not a bug in Vitest.", ); err.name = 'EnvironmentTeardownError'; throw err;🤖 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 `@packages/loader-fs/test/fixtures/loadfile/teardown-error.js` around lines 1 - 10, The test fixture teardown-error.js uses a JavaScript extension but the monorepo follows a TypeScript-first approach for consistency across all packages. Rename the file teardown-error.js to teardown-error.ts to align with the coding guidelines and maintain consistency with the rest of the fixture files in the monorepo.Source: Coding guidelines
packages/loader-fs/src/index.ts (1)
29-29: 💤 Low valueConsider extracting the depth limit as a named constant.
The hardcoded
5in the loop condition could be extracted as a named constant (e.g.,MAX_CAUSE_DEPTH = 5) for better clarity and maintainability.♻️ Optional refactor
+const MAX_CAUSE_DEPTH = 5; + // Detect vitest's "environment was torn down" error, raised when a dynamic // import() resolves after the test environment that issued it has already been // torn down. It is reported by name (`EnvironmentTeardownError`) and/or message, // possibly nested behind a `cause`, so walk a short cause chain. This condition // exists only in the vitest runner during teardown, never in production. function isEnvironmentTeardownError(e: unknown): boolean { let cur = e as { name?: unknown; message?: unknown; cause?: unknown } | undefined | null; - for (let depth = 0; cur && depth < 5; depth++) { + for (let depth = 0; cur && depth < MAX_CAUSE_DEPTH; depth++) { if (cur.name === 'EnvironmentTeardownError') return true; if (typeof cur.message === 'string' && cur.message.includes('after the environment was torn down')) { return true; } cur = cur.cause as typeof cur; } return false; }🤖 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 `@packages/loader-fs/src/index.ts` at line 29, The for loop in the file has a hardcoded depth limit value of 5 in the condition depth < 5. Extract this magic number as a named constant (e.g., MAX_CAUSE_DEPTH with a value of 5) at the module level, and then replace the hardcoded 5 in the for loop condition with this constant reference. This improves code readability and makes it easier to adjust the depth limit in the future.packages/loader-fs/test/fixtures/loadfile/normal-error.js (1)
1-4: 💤 Low valueConsider using
.tsor.mjsextension for test fixtures.Like
teardown-error.js, this fixture could use a.tsor.mjsextension to align with the monorepo's TypeScript-first guidelines.♻️ Optional refactor to TypeScript
Rename to
normal-error.ts:-'use strict'; - // A genuine load-time failure must still propagate (wrapped) from loadFile. throw new Error('boom: real load failure');🤖 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 `@packages/loader-fs/test/fixtures/loadfile/normal-error.js` around lines 1 - 4, Rename the test fixture file `normal-error.js` to `normal-error.ts` to align with the monorepo's TypeScript-first guidelines and maintain consistency with other similar fixture files in the codebase. The file content can remain the same, only the file extension needs to be changed from `.js` to `.ts`.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 `@packages/loader-fs/src/index.ts`:
- Line 29: The for loop in the file has a hardcoded depth limit value of 5 in
the condition depth < 5. Extract this magic number as a named constant (e.g.,
MAX_CAUSE_DEPTH with a value of 5) at the module level, and then replace the
hardcoded 5 in the for loop condition with this constant reference. This
improves code readability and makes it easier to adjust the depth limit in the
future.
In `@packages/loader-fs/test/fixtures/loadfile/normal-error.js`:
- Around line 1-4: Rename the test fixture file `normal-error.js` to
`normal-error.ts` to align with the monorepo's TypeScript-first guidelines and
maintain consistency with other similar fixture files in the codebase. The file
content can remain the same, only the file extension needs to be changed from
`.js` to `.ts`.
In `@packages/loader-fs/test/fixtures/loadfile/teardown-error.js`:
- Around line 1-10: The test fixture teardown-error.js uses a JavaScript
extension but the monorepo follows a TypeScript-first approach for consistency
across all packages. Rename the file teardown-error.js to teardown-error.ts to
align with the coding guidelines and maintain consistency with the rest of the
fixture files in the monorepo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56bd7a2b-397d-4f79-9ee3-ef8e288b5634
📒 Files selected for processing (4)
packages/loader-fs/src/index.tspackages/loader-fs/test/fixtures/loadfile/normal-error.jspackages/loader-fs/test/fixtures/loadfile/teardown-error.jspackages/loader-fs/test/loader_fs.test.ts
killagu
force-pushed
the
fix/mock-teardown-import-race
branch
from
18afdaa to
92a43fc
Compare
|
Refined ( |
killagu
force-pushed
the
fix/mock-teardown-import-race
branch
from
92a43fc to
631404a
Compare
`Test (ubuntu-latest, 24)` flakily failed with `EnvironmentTeardownError: Cannot load '<file>' ... after the environment was torn down. This is not a bug in Vitest.` (e.g. loading `duplicate-proto-name-app`'s `AppController.ts`, or `logrotator`'s `rotate_by_file.ts`). ## Root cause The framework loads files via dynamic `import()` through `RealLoaderFS.loadFile`. Tracing the dangling load shows it is a *legitimate, fully-awaited boot-time load* — e.g. the schedule plugin loading schedule files in `configDidLoad` (`loadFile <- getExports <- ScheduleLoader <- loadSchedule <- Scheduler.init/ScheduleWorker.init <- configDidLoad`). There is no fire-and-forget async leak. It outlives teardown because of the suite's `isolate: false` (threads) config: an `import()` issued while constructing an app in one test file's module-runner environment can have follow-up settling (tsx transform / transitive resolution) that completes after vitest tears that environment down to move to the next file. vitest itself labels this "not a bug in Vitest". `loadFile` wrapped and rethrew it, so it became an unhandled rejection that, under `isolate: false`, is attributed to whatever unrelated test file is running — failing it at random. Same class of teardown race as #5978, different symptom. ## Fix Since the load is correct and awaited, there is nothing to fix at the source; the only artifact is the benign vitest-internal error. Detect it in `loadFile` (by `name === 'EnvironmentTeardownError'` and/or the "after the environment was torn down" message, walking the `cause` chain) and resolve `undefined` instead of throwing. Loaders already treat an empty module as "no exports", so this is a safe no-op; genuine load-time failures still propagate unchanged. The detection is gated on `process.env.VITEST`, so it is unmistakably a test-runner-only accommodation that can never alter production loader behavior. Verified against the `@eggjs/mock` suite: on `next` it fails with this teardown error; with the fix that error is gone. Adds regression coverage for both the swallow and the still-throws-on-real-error paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
fix/mock-teardown-import-race
branch
from
631404a to
ecd2b4a
Compare
Motivation
Test (ubuntu-latest, 24)flakily fails with:Observed in CI loading
tegg/plugin/controller'sduplicate-proto-name-app/.../AppController.ts, and reproduced locally in the@eggjs/mocksuite loadinglogrotator'srotate_by_file.ts— failing unrelated tests (e.g.mock_httpclient_next"should auto restore after each case").Root cause (traced)
The framework loads files via dynamic
import()throughRealLoaderFS.loadFile. Instrumenting the dangling load shows it is a legitimate, fully-awaited boot-time load — e.g. the schedule plugin loading schedule files duringconfigDidLoad:Every link awaits:
loadScheduleawaits the loader,configDidLoadawaitsloadSchedule,ready()awaitsconfigDidLoad. Timers /serverDidReady/ strategystart()do not import. So there is no fire-and-forget async leak to fix at the source.It outlives teardown because of the suite's
isolate: false(threads) config: animport()issued while constructing an app in one test file's module-runner environment can have follow-up settling (tsx transform / transitive resolution) that completes after vitest tears that environment down to move to the next file. vitest itself labels this"This is not a bug in Vitest".loadFilewrapped and rethrew it, so it became an unhandled rejection that, underisolate: false, gets blamed on whatever unrelated test file is running. Same class of teardown race addressed for a different symptom in #5978.Fix
Because the load is correct and awaited, there's nothing to fix at the source — the only artifact is the benign vitest-internal error. Detect it in
loadFile(byname === 'EnvironmentTeardownError'and/or the"after the environment was torn down"message, walking thecausechain) and resolveundefinedinstead of throwing.process.env.VITESTso it is unmistakably a test-runner-only accommodation that can never alter production loader behavior.Test evidence
next,pnpm vitest run plugins/mock/testfails with theEnvironmentTeardownError; with this fix that error is gone. (A separate, pre-existingmock errorcross-file leak fromapp.test.ts/agent.test.tsremains — different root cause, out of scope; it was previously masked by the teardown error.)loader-fsregression tests assertloadFileresolvesundefinedfor a teardown-style import error and still throws (wrapped) for a genuine load failure. Full@eggjs/loader-fssuite: 4 passed.oxlint --type-aware,oxfmt --check, andtsgo --noEmitclean on the changed files.🤖 Generated with Claude Code
Summary by CodeRabbit
undefinedinstead of failing.