Uh oh!
There was an error while loading. Please reload this page.
[vitest] Write vitest workflow data together with other workflow data, fixes observability for test runs - #1359
Merged
Conversation
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
🦋 Changeset detectedLatest commit: 39b957c The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (55 failed)mongodb (3 failed):
redis (2 failed):
turso (50 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
Uh oh!
There was an error while loading. Please reload this page.
…uplicate data in streams when a tag is set.
This commit fixes the issue reported at packages/world-local/src/streamer.ts:316
## Bug Analysis
When `tag` is set (e.g., `vitest-0`), `readFromStream` lists chunk files using both `.bin` and `.{tag}.bin` extensions. The `listFilesByExtension` function strips the extension from filenames and returns the base names.
For a file `stream-chnk_ABC.vitest-0.bin` on disk:
1. `listFilesByExtension(chunksDir, '.bin')` → matches (ends with `.bin`) → returns `stream-chnk_ABC.vitest-0`
2. `listFilesByExtension(chunksDir, '.vitest-0.bin')` → matches (ends with `.vitest-0.bin`) → returns `stream-chnk_ABC`
Both entries are added to `fileExtMap` as separate keys because their stripped names are different:
- `stream-chnk_ABC.vitest-0` → `.bin` - `stream-chnk_ABC` → `.vitest-0.bin`
Both pass the `file.startsWith(`${name}-`)` filter and both appear in `chunkFiles`. When processing:
- Entry 1 (`stream-chnk_ABC.vitest-0`): rawChunkId=`chnk_ABC.vitest-0`, chunkId=`chnk_ABC` (after tag strip), reads file `stream-chnk_ABC.vitest-0.bin` and enqueues data
- Entry 2 (`stream-chnk_ABC`): rawChunkId=`chnk_ABC`, chunkId=`chnk_ABC` (no tag to strip), reads file `stream-chnk_ABC.vitest-0.bin` and enqueues same data AGAIN
The `deliveredChunkIds` set only tracks event-delivered chunks (populated in the `chunkListener`), not disk-read chunks, so there's no deduplication for the disk reads.
## Fix
When `tag` is set, filter out entries from `binFiles` that end with `.{tag}` since those same physical files will be properly discovered and handled by the tagged listing (`taggedBinFiles`). This is done by checking `if (tag && f.endsWith(tagSuffix)) continue;` when iterating over `binFiles`.
This ensures each physical file appears exactly once in `fileExtMap` — either via the plain `.bin` listing (for untagged files) or via the `.{tag}.bin` listing (for tagged files), but never both.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: VaguelySerious <mittgfu@gmail.com>VaguelySerious
marked this pull request as ready for review
March 12, 2026 23:14
TooTallNate
requested changes
Mar 13, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Good PR — the tagged-filename approach is well thought out and the PR description is excellent. One blocking issue around hook token constraint cleanup in clear(), plus a minor comment nit.
Summary of review:
Blocking: Tagged
clear()does not clean up hook token constraint files. These files are written untagged ({sha256}.json) intohooks/tokens/, solistTaggedFileswill never match them. Orphaned constraints will causehook_conflictevents in subsequent test runs.Nit: Misleading comment in
deleteAllWaitsForRun— says "strip tag" but doesn't actually callstripTag.
Everything else looks solid: the stripTag regex is correct (entity IDs never contain dots), getObjectCreatedAt correctly strips tags before ULID extraction, the stream dedup fix is well-analyzed and correct, the ENOENT race fix in ensureDataDir is good, and the test coverage is thorough.
| 'steps', | ||
| 'events', | ||
| 'hooks', | ||
| 'hooks/tokens', |
Member
There was a problem hiding this comment.
Bug (blocking): Token constraint files in hooks/tokens/ are written as {sha256}.json (untagged — see events-storage.ts line 603), so listTaggedFiles(fullDir, tag) here will never match them. After a tagged clear(), orphaned constraint files remain and will block reuse of the same hook tokens in subsequent test runs (writeExclusive will see the old constraint and produce hook_conflict events instead of hook_created).
Two options:
- Write constraint files with the tag too (e.g.,
{hash}.{tag}.json) and update the conflict-detection path to also check the tagged filename. - In the tagged
clear()path, read each tagged hook file to extract its token, compute the hash, and explicitly delete the matching constraint file (mirroring whathook_disposeddoes on line 678-684).
| for (const file of files) { | ||
| // Strip tag before prefix check — fileIds may contain tags | ||
| // e.g., "wrun_ABC-corrId.vitest-0" still starts with "wrun_ABC-" |
Member
There was a problem hiding this comment.
Nit: The comment says "strip tag before prefix check" but the code does not actually call stripTag. It works correctly because the tag is a suffix and startsWith only checks the beginning — but the comment is misleading. Consider:
Suggested change
| // e.g., "wrun_ABC-corrId.vitest-0" still starts with "wrun_ABC-" | |
| // fileIds may contain tag suffixes (e.g., "wrun_ABC-corrId.vitest-0") | |
| // but startsWith still matches correctly since the tag is a suffix. |
pranaygp
left a comment
Contributor
There was a problem hiding this comment.
Good approach overall — the tagged-filename strategy is elegant and avoids the complexity of multi-directory discovery. The PR description is excellent. Agreeing with TooTallNate's changes-requested review, plus a couple additional items below.
Existing issue (agreeing with @TooTallNate):
- The
hooks/tokens/constraint file cleanup in taggedclear()is a real bug. See my inline comment for a concrete fix suggestion.
Additional findings:
- Changeset missing
@workflow/vitest— The vitest package has behavioral changes (switched from isolated.workflow-vitest/data/{poolId}to shared.workflow-datawith tags, addedinitDataDirpre-creation). If it's a published package, it should be included in the changeset. - Test coverage gap — The
tagged clear()tests don't exercise hooks with token constraints, which is how the constraint file bug went undetected. Consider adding a test that creates a hook, callsclear(), and verifies the constraint file is also removed.
| files.map((f) => deleteJSON(path.join(fullDir, f))) | ||
| ); | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
+1 on TooTallNate's blocking comment here. The constraint files in hooks/tokens/ are written untagged at events-storage.ts:603 (${hashToken(hookData.token)}.json), so listTaggedFiles will never match them.
I'd suggest option 2 from his comment (reading tagged hooks to extract tokens) since it keeps the constraint file format stable and doesn't require changes to the conflict-detection path. Something like:
// In the tagged clear() path, after deleting tagged hook files:consthooksDir=path.join(basedir,'hooks');consttaggedHooks=awaitlistTaggedFiles(hooksDir,tag);for(consthookFileoftaggedHooks){consthookPath=path.join(hooksDir,hookFile);consthook=awaitreadJSON(hookPath,HookSchema);if(hook){constconstraintPath=path.join(hooksDir,'tokens',`${hashToken(hook.token)}.json`);awaitdeleteJSON(constraintPath);}}Note: this read-then-delete needs to happen before the hook files themselves are deleted in the entityDirs loop.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@workflow/world-local": patch | |||
Contributor
There was a problem hiding this comment.
Should @workflow/vitest also be included here? The vitest package has a behavioral change: it switches from writing to .workflow-vitest/data/{poolId}/ to the shared .workflow-data/ directory with tag-based isolation. The initDataDir pre-creation call is also new. If @workflow/vitest is published, consumers should see this in the changelog.
| expect(await fs.readdir(stepsDir)).toHaveLength(0); | ||
| await world.close?.(); | ||
| }); |
Contributor
There was a problem hiding this comment.
This test suite is thorough for runs/events/steps, but the gap here is that it doesn't test clear() with hooks that have token constraint files. Adding a test that:
- Creates a run + hook (which writes a constraint file in
hooks/tokens/) - Calls
world.clear() - Asserts the constraint file was also deleted
would have caught the bug TooTallNate identified.
- Fix tagged clear() to also delete hook token constraint files by
reading tagged hook files to extract tokens before deletion
- Fix misleading comment in deleteAllWaitsForRun (says "strip tag"
but doesn't call stripTag — the prefix check works because tags
are suffixes)
- Add @workflow/vitest to changeset (behavioral change: switches
from .workflow-vitest/data/{poolId} to shared .workflow-data)
- Add test for clear() with hooks verifying constraint file cleanup
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>TooTallNate
approved these changes
Mar 13, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Both issues from my previous review are fixed:
Hook token constraint cleanup —
clear()now reads each tagged hook file to extract its token, computes the hash, and deletes the matching constraint file before deleting the hook files. Correct ordering and the newshould clear hook token constraint filestest covers it.Misleading comment — Updated to accurately describe the
startsWithbehavior with tagged fileIds.Changeset — Now includes
@workflow/vitestalongside@workflow/world-local.
The Windows E2E failure is unrelated (flaky should rebuild on workflow change test timing out).
LGTM.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Problem
Vitest workflow tests write run data to
.workflow-vitest/data/{poolId}/, while normal local development writes to.workflow-data/. The observability tools (CLI and web UI) only discover runs in.workflow-data/(and.next/workflow-data), so vitest-created runs are invisible to observability.Vitest calls
world.clear()before each test file to ensure clean state. The existingclear()doesrm -rfon the entire data directory, which is why vitest uses a separate directory: clearing.workflow-data/would destroy dev server runs.Goal: Make vitest runs visible to observability without losing the ability to selectively clear test data.
Rejected Approaches
1. Multi-directory discovery
Make
findWorkflowDataDir()return multiple directories and aggregate runs across.workflow-dataand.workflow-vitest/data/*.Rejected because: Requires the observability layer to merge paginated queries across multiple directories, adding significant complexity to listing, pagination, and cursor management. Every consumer of the storage API would need to handle multi-source aggregation.
2. Vitest subdirectory inside
.workflow-dataWrite vitest data to
.workflow-data/vitest/{poolId}/and have observability recurse into subdirectories.Rejected because: This is essentially the multi-directory problem in disguise. The filesystem storage's flat directory structure (
runs/,steps/,events/) doesn't support nested data dirs. Would require significant refactoring ofpaginatedFileSystemQueryand all storage modules.3. Run ID prefixes
Give all vitest-created runs a known prefix (e.g.,
vitest-0-*). Replaceworld.clear()with a selective delete that removes runs matching the prefix.Rejected because: Run IDs are generated deep in the runtime as ULIDs. Injecting prefixes would require changes across the entire runtime, and the prefix would leak test infrastructure concerns into production-format IDs.
4a. Timestamp-based clearing
Write to
.workflow-datadirectly. Track a start timestamp and clear all entities created after that timestamp.Rejected because: Could accidentally delete runs created by a concurrent dev server during the test window. Clearing on setup (before tests) is more reliable than teardown, but the "created after" approach doesn't work for pre-test cleanup since you don't know which runs are from the previous test session.
4b. Manifest-based tracking
Write standard filenames but maintain a side-car manifest file listing which files belong to each vitest pool.
Rejected because: Adds a second source of truth. If a test crashes before the manifest is updated, orphaned files accumulate. The manifest must be kept in sync with every write and delete operation.
Chosen Approach: Tagged Filenames
Concept
When
createLocalWorld({ tag: 'vitest-0' })is called, all files are written with the tag embedded in the filename:{id}.{tag}.jsonfilenames.jsonfiles naturally, since both tagged and untagged files end in.jsonclear()only deletes files matching*.{tag}.json, preserving untagged filesWhy this works
Entity IDs never contain dots (ULIDs are alphanumeric, step IDs are
step_N, etc.), so the first dot after the entity ID always marks the tag boundary.paginatedFileSystemQueryalready lists all.jsonfiles vialistJSONFiles, so tagged files are automatically included in listings. The tag in the filename doesn't affect pagination since the actual entity data (parsed from JSON) contains the real IDs and timestamps.getObjectCreatedAtstrips the tag before ULID extraction, so filename-based cursor optimization works correctly with tagged files.Vitest pool isolation is achieved by tag uniqueness (
vitest-0,vitest-1, etc.). Each pool'sclear()only deletes its own tagged files.Key implementation details
taggedPath(basedir, entityDir, fileId, tag?)builds write pathsreadJSONWithFallback(basedir, entityDir, fileId, schema, tag?)tries tagged path first, falls back to untaggedstripTag(fileId)removes the tag suffix using the pattern/\.[a-zA-Z][a-zA-Z0-9-]*$/listTaggedFiles(dirPath, tag)finds files matching*.{tag}.jsonfor clearing{name}-{chunkId}.{tag}.binChanges to vitest integration