Fix Vitest project isolation and paginated recovery filtering#1818
Closed
tomdale wants to merge 7 commits into
Closed
Fix Vitest project isolation and paginated recovery filtering#1818tomdale wants to merge 7 commits into
tomdale wants to merge 7 commits into
Conversation
Scope local-world startup recovery to the active tag so Vitest workers do not re-enqueue runs owned by other workers. Also add an opt-out for recovery, disable it in the Vitest harness, and allow callers to override the test data and bundle directories. Add regression coverage for tagged recovery, recovery opt-out, and custom Vitest directory configuration.
🦋 Changeset detectedLatest commit: c9282a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (1 failed)nextjs-webpack (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Member
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| path.join(os.tmpdir(), 'workflow-vitest-build-') | ||
| ); | ||
| tempDirs.push(rootDir); | ||
| const cwd = '/repo/app'; |
Member
There was a problem hiding this comment.
AI Review: Blocking
The new tests in this file hardcode POSIX-style absolute paths (/repo/app, /tmp/workflow-vitest-root, etc.) and compare them directly against the output of resolve(...) inside options.ts. On Windows, path.resolve('/repo/app') expands to the current drive (e.g. D:\repo\app), so the assertions diverge and the test file fails.
This is why Unit Tests (windows-latest) is failing on this PR (4 failed in src/index.test.ts) while it passes on main. Lines that bleed into the flake: 108 (cwd), 186 (cwd), 226–229 / 236–239 / 265–268 (the __workflowVitestOptions objects passed to global-setup and setup-file).
Either compute the expected values with path.resolve(...) / path.join(...) at assertion time, or anchor the fixtures to os.tmpdir() (the filesystem-touching tests above already do this). Without that, the Windows unit-test lane stays red.
Member
There was a problem hiding this comment.
(AI) Fixed in aecde1a — the test fixtures now pass absolute paths through path.resolve(...) / path.join(...), so the expected values match the internal resolve() output on both POSIX and Windows.
| * Directory for generated workflow and step bundles. | ||
| * Defaults to `<rootDir>/.workflow-vitest`. | ||
| */ | ||
| outDir?: string; |
Member
There was a problem hiding this comment.
AI Review: Blocking
This adds three new user-facing options to WorkflowTestOptions (rootDir, dataDir, outDir), and the workflow() signature below now accepts a WorkflowTestOptions argument — previously it took none. The reference docs under docs/content/docs/api-reference/vitest/index.mdx still document only cwd on WorkflowTestOptions and still show workflow() invoked with no arguments.
Per the repo convention that user-facing changes in vercel/workflow are documented in docs, please update that page to: (1) add rootDir, dataDir, outDir to the WorkflowTestOptions table, noting how relative paths resolve against cwd and the <rootDir>/.workflow-data / <rootDir>/.workflow-vitest defaults; (2) mention that workflow() accepts an optional WorkflowTestOptions and show a usage snippet for a workspace with a non-standard layout — that is the motivating use case for this PR, so it's worth surfacing.
Member
There was a problem hiding this comment.
(AI) Fixed in aecde1a — the @workflow/vitest reference page now documents rootDir, dataDir, outDir with their resolution rules and default locations, notes that workflow() accepts an optional WorkflowTestOptions, and shows a usage snippet for a non-standard layout.
| storage.runs.list({ | ||
| ...params, | ||
| fileIdFilter: (fileId: string) => hasTag(fileId, tag), | ||
| } as any)) as typeof storage.runs.list, |
Member
There was a problem hiding this comment.
AI Review: Nit
This wrapper re-implements storage.runs.list with two as any casts (on params and on the call itself) and therefore bypasses the outer instrumentObject('world.runs', ...) span for the recovery call — the inner storage.runs.list(...) call is still instrumented, so telemetry is preserved, but the shape is awkward.
A cleaner alternative would be to plumb fileIdFilter through an internal extension of ListWorkflowRunsParams (still kept out of the public Storage['runs']['list'] surface) so the wrapper can drop the double as any. Not blocking — the code is internal and correct — just flagging for future cleanup.
Member
There was a problem hiding this comment.
(AI) Addressed in aecde1a — plumbed fileIdFilter through a local-only LocalListWorkflowRunsParams (and LocalRunsStorage / LocalStorage) that structurally assigns to Storage['runs'] at the reenqueueActiveRuns boundary. The recovery wrapper in createLocalWorld no longer needs the double as any, and the option stays off the public @workflow/world surface.
| world = createLocalWorld({ | ||
| dataDir: join(cwd, '.workflow-data'), | ||
| dataDir, | ||
| recoverActiveRuns: false, |
Member
There was a problem hiding this comment.
AI Review: Note
Good call registering handlers before world.start?.() — defensive against any future change where recoverActiveRuns gets re-enabled here (or a caller of setupWorkflowTests passes it through). Since the harness hardcodes recoverActiveRuns: false today the ordering is defensive, not load-bearing; a short comment noting that register-before-start is intentional would help prevent a later cosmetic reorder from silently reintroducing the race this PR just fixed.
Member
There was a problem hiding this comment.
(AI) Added an inline comment in aecde1a noting that register-before-start is intentional and that reordering would reintroduce the race if recoverActiveRuns is ever re-enabled or passed through by a caller.
Member
|
I like the approach overall and would love to ship this after fixing the blocking issues |
- Use path.resolve() in vitest test fixtures so assertions match on Windows - Document rootDir/dataDir/outDir and workflow() options in docs - Note intent of register-before-start ordering in setupWorkflowTests - Plumb fileIdFilter via internal LocalListWorkflowRunsParams type to drop the double `as any` on the recovery wrapper Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Wielander <mittgfu@gmail.com>
VaguelySerious
force-pushed
the
tomdale/local-world-isolation
branch
from
166aa47 to
aecde1a
Compare
VaguelySerious
approved these changes
May 2, 2026
…next test The 'should include steps discovered from workflow imports' test creates a workflow + step file pair, lets the deferred builder discover them, and restores cleanup via afterEach. Since #1796 the generated step route bundle imports the discovered step source directly (no copy), so when the source is deleted the bundle is left with a literal `import '../workflows/discovered-via-workflow-step.ts'` until the deferred builder rebuilds and `filterExistingFiles` prunes it. On Windows the rebuild can lag the deletion, leaving the step route bundle unable to compile for the next test file (which shares the same dev server). Every step request then returns 500 and the e2e suite hangs until the 30-min job timeout. The bug was masked on main by an earlier dev test failure that short-circuits the suite. Tear the test down in-test instead of via afterEach: restore the api file, delete the workflow + step files, and poll the manifest until the discovered step is gone before returning. Also reorder afterEach to restore content before deleting files, so the dev server never sees an api import pointing at a missing module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Before this change,
@workflow/vitestpassed custom test directories through process-wide env vars, so Vitest workspace projects and config reloads could leak one project's paths into another.world-localalso added tagged recovery for Vitest workers, but its paginated filesystem query dropped the tag filter after the first page, which could re-enqueue runs owned by other workers in shared local data.This branch moves the Vitest harness to project-scoped provided context.
workflow()resolvescwd,rootDir,dataDir, andoutDironce, passes them through Vitest's per-project context, andglobal-setupplussetup-fileread that resolved state when building bundles and creating the local test world. The package metadata and lockfile now also declare the Vitest test dependency and script used by the package.On the recovery side,
world-localnow keepsfileIdFilterapplied while paginating and uses that path to scope startup recovery to the active tag. It also adds an explicitrecoverActiveRunsopt-out and disables recovery in the Vitest harness, so worker startup does not re-enqueue stale runs before direct handlers are registered.Testing
pnpm exec vitest run packages/vitest/src/index.test.ts packages/world-local/src/fs.test.ts packages/world-local/src/reenqueue.test.tspnpm exec tsc -p packages/vitest/tsconfig.json --noEmitpnpm exec tsc -p packages/world-local/tsconfig.json --noEmit