fix(core): isolate Vite cache for integration test servers - #2413
Conversation
|
|
All contributors have signed the CLA ✍️ ✅ |
github-actions
Bot
added
area/core
size/S
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly fixes the Vite cache race in concurrent integration test servers. The approach is sound: each createTestServer already gets a private workDir, so putting .vite-cache under that root and passing it to Vite's cacheDir naturally isolates the dependency optimizer without disturbing the symlinked node_modules sharing. Cleanup is handled by the existing rmSync(workDir).
I checked the three changed files and traced the new EMDASH_TEST_VITE_CACHE variable from server.ts through the fixture's astro.config.mjs to the regression assertion in concurrent-servers.test.ts. There are no other occurrences. The change is test-only, so none of the production conventions (SQL, auth, locale filtering, logged-out query counts, i18n) are affected. No changeset is needed for a test-only fix.
The new assertion verifies the isolation directly: realpathSync confirms the per-server .vite-cache directories resolve to different physical paths. The 120-second timeout on the slow integration test is appropriate for cold dev-server startup.
I found no bugs, regressions, or convention violations. Good to merge.
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
github-actions
Bot
added
cla: signed
review/approved
MA2153
left a comment
MA2153
left a comment
There was a problem hiding this comment.
Thank you for tackling this! Just a couple notes on the regression test.
| // back to the same physical cache directory. | ||
| const viteCacheA = realpathSync(join(a.cwd, ".vite-cache")); | ||
| const viteCacheB = realpathSync(join(b.cwd, ".vite-cache")); | ||
| expect(viteCacheA).not.toBe(viteCacheB); |
There was a problem hiding this comment.
This seems to be a tautology. Two lines above, the test already asserts a's cwd and b's cwd are different. What actually makes the test fail appears to be realpathSync throwing. You can instead try something like:
-import { realpathSync } from "node:fs";
+import { existsSync, readdirSync } from "node:fs";
@@
- // The Vite dependency optimizer must not follow both node_modules symlinks
- // back to the same physical cache directory.
- const viteCacheA = realpathSync(join(a.cwd, ".vite-cache"));
- const viteCacheB = realpathSync(join(b.cwd, ".vite-cache"));
- expect(viteCacheA).not.toBe(viteCacheB);
+ // Vite must write the dependency optimizer's output into each server's own
+ // cache dir, not follow the shared node_modules symlink to one physical copy.
+ for (const cwd of [a.cwd, b.cwd]) {
+ const cacheDir = join(cwd, ".vite-cache");
+ expect(existsSync(cacheDir), `${cacheDir} was never created`).toBe(true);
+ expect(readdirSync(cacheDir)).toContain("deps");
+ }| expect(slugs).toContain("pages"); | ||
| } | ||
| }); | ||
| }, 120_000); |
There was a problem hiding this comment.
This might be too generous. I am not sure what's the appropriate number to set it to, but other tests seem to run comfortably with a 30s timeout.
github-actions
Bot
added
review/needs-rereview
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The outer timeout must exceed the helper's startup-only budget so accepted and failure paths can finish setup, report errors, and clean up.
| expect(slugs).toContain("pages"); | ||
| } | ||
| }); | ||
| }, 90_000); |
There was a problem hiding this comment.
createTestServer itself allows 90 seconds just for the server to bind, then still performs setup and seeding. With this identical 90-second Vitest limit, a server that binds near the helper deadline—or exhausts it—makes Vitest abort before setup/error reporting and cleanup can finish, potentially leaving the child alive. Set this above the helper budget (for example, restore 120 seconds), or pass shorter timeout values to these calls so the outer limit retains cleanup headroom.
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
|
Restored the outer timeout to 120s while keeping the helper at 90s, so cleanup has headroom. Focused test and lint/format checks pass thanks for catching this. |
github-actions
Bot
added
review/approved
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
3 participants
What does this PR do?
Fixes #2247
Each integration test server already receives its own copied
workDir, which isolates Astro's project lock. The fixture intentionally sharesnode_modulesthrough a symlink to the workspace dependency tree so server startup remains fast and parallel.Vite followed that symlink and inherited one physical
node_modules/.vitedependency-optimizer cache. Parallel servers could therefore rewrite the same cache concurrently, producingENOTEMPTYfailures innode_modules/.vite/depsand making the integration suite flaky.This change assigns each server a private
.vite-cachedirectory under its ownworkDir, passes it through the test-onlyEMDASH_TEST_VITE_CACHEenvironment variable, and maps it to Astro/Vite'scacheDir. Sharednode_modulesand parallelism are preserved. The existing cleanup removes the cache with the normal per-serverworkDir.The regression test requires each server's private
.vite-cachedirectory to exist and contain actual optimizer/cache file output. This catches fallback to the sharednode_modules/.vitecache without relying on cwd inequality, while retaining the concurrent server startup and seeded-content checks.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) ? focused concurrent-server regression and integration/CLI coverage passpnpm formathas been run ? the three changed files were formatted and checked directly with Oxfmt and Prettier; the repository-wide format baseline was not rewrittenmessages.pochanges except in translation PRs ? n/a, no user-facing stringsAI-generated code disclosure
Screenshots / test output
RED -> GREEN
it(...)only; 90s matches the helper startup budget while reducing the previous 120s allowance; global Vitest timeout behavior is unchanged.Validation
pnpm typecheck: passed across the workspace packages.pnpm lint: passed.pnpm lint:quick: passed with 0 diagnostics across 2,310 files.git diff --check: passed.pnpm install --frozen-lockfile: passed; the lockfile remained unchanged.pnpm --filter emdash build: passed in 64 seconds.vitest.integration.config.tspassed 1 test in 56.6 seconds; the current test timeout is 90 seconds (the recorded run used--testTimeout 120000).pnpm buildwas attempted but exceeded the local five-minute tool ceiling while buildingpackages/core; it reported no build error before the timeout, and the narrower core build passed.execFile("pnpm", ...)spawn pnpm ENOENTissue and Windows symlink permissions, so the real-server regression was run under WSL/Linux.