Add world.snapshots storage interface (local, postgres, vercel) - #3250
Open
TooTallNate wants to merge 3 commits into
Open
Add world.snapshots storage interface (local, postgres, vercel)#3250TooTallNate wants to merge 3 commits into
TooTallNate wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: d2a115b The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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
🧪 E2E Test Results❌ Some tests failed ❌ Failed E2E Tests▲ Vercel Production (372 failed)astro-node (12 failed):
astro-quickjs (16 failed):
example-node (15 failed):
example-quickjs (12 failed):
express-node (11 failed):
express-quickjs (22 failed):
fastify-node (10 failed):
fastify-quickjs (17 failed):
hono-node (12 failed):
hono-quickjs (18 failed):
nextjs-turbopack-node (20 failed):
nextjs-turbopack-quickjs (18 failed):
nextjs-webpack-node (25 failed):
nextjs-webpack-quickjs (25 failed):
nitro-node (14 failed):
nitro-quickjs (12 failed):
nuxt-node (14 failed):
nuxt-quickjs (17 failed):
sveltekit-node (17 failed):
sveltekit-quickjs (30 failed):
vite-node (7 failed):
vite-quickjs (28 failed):
💻 Local Development (2 failed)astro-stable-node (1 failed):
sveltekit-stable-node (1 failed):
📦 Local Production (1 failed)sveltekit-stable-node (1 failed):
📋 Other (61 failed)e2e-vercel-prod-nest-node (8 failed):
e2e-vercel-prod-nest-quickjs (21 failed):
e2e-vercel-prod-tanstack-start-node (18 failed):
e2e-vercel-prod-tanstack-start-quickjs (14 failed):
E2E Test SummarySummary
Details by Category❌ ▲ Vercel Production
❌ 💻 Local Development
❌ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 📋 Other
✅ vercel-multi-region
|
Contributor
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
TooTallNate
force-pushed
the
quickjs-vm-snapshots
branch
from
July 31, 2026 21:15
a4e1f58 to
c935825
Compare
…l implementations
…torage
The security bot flagged path traversal: an unvalidated runId was
interpolated directly into {basedir}/snapshots paths, letting ../ (or /,
\, NUL, absolute fragments) escape the snapshots dir on save/load/delete.
Apply the same two-layer scheme as the other world-local storages:
assertSafeEntityId as the primary defense plus resolveWithinBase
containment at the join. Hostile-runId test covers all three operations.
TooTallNate
force-pushed
the
quickjs-vm-snapshots
branch
from
July 31, 2026 23:09
ff2c6e4 to
968cfa1
Compare
Contributor
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 152131ms → this run 129941ms (Δ -22190ms, -15%) 1020 steps (queue-hop) Cumulative STSO time: main 2084ms → this run 3081ms (Δ +997ms, +48%) 📜 Previous results (1)968cfa1Fri, 31 Jul 2026 23:24:59 GMT · run logs
ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
pranaygp
reviewed
Aug 3, 2026
pranaygp
left a comment
pranaygp
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the incremental diff (16 files, +838). Mergeable with fixes — nothing here runs until #3251, but three inline findings become live correctness bugs the moment it does, and the changeset/semver one ships breakage to community worlds on merge.
Beyond the inline comments:
- Orphaned snapshots on postgres cleanup: the legacy
run_cancelledpath instorage.tsdeletes hooks and waits but not snapshots; there are zero FKs/cascades inschema.tsand no retention job, and rows are 5–15 MBbytea— unbounded growth. Other cleanup sites worth checking:storage.ts:948, 1000, 1045, 1601. - All three vercel paths bypass
makeRequest, losing transient-error →WorkflowWorldError{code:'TRANSPORT'}mapping (retryability),getRequestTimeoutMs(), and the http logging. Theundici.request()choice forsaveis well-justified (undici#3288 — Buffer body survives RetryAgent retries) but argues for teachingmakeRequesta raw-body mode, not three hand-rolled paths. - Older-server compat:
load's 404→null degrades gracefully by accident, butsave/deletethrow against a workflow-server without the endpoints — #3251 must treatsaveas best-effort or this becomes a hard version coupling. - Minor: world-local snapshots is the only sub-storage not given the
tag(breaks multi-world basedir isolation); noload/deletetest coverage on the vercel client (the 503-retry save test is genuinely good); nothing exercises the postgres table beyond the migration applying;eventsCursorrides an HTTP header unencoded (constrains future cursor formats). - Migration note: it's
0018_add_snapshots_table.sql(the PR description says the stack docs referenced 0016).
Path-traversal hardening in the runId-validation follow-up commit is solid (two independent layers, well-tested) — no findings there.
| metadata: SnapshotMetadata | ||
| ): Promise<void> { | ||
| await ensureDir(snapshotsDir); | ||
| await Promise.all([ |
Contributor
There was a problem hiding this comment.
Torn save pair: write() is atomic per file (temp + rename), but this Promise.all of two renames is not — a crash between them, or a concurrent load interleaving, pairs a .bin from one suspension with an eventsCursor from another. That's the worst failure mode for this feature: restore replays from the wrong log position and silently diverges instead of erroring. Suggest a single atomic write (metadata header + data in one file) or data-then-metadata with the metadata carrying a digest of the data it describes, as the commit marker.
| const data = new Uint8Array(buffer); | ||
|
|
||
| const eventsCursor = | ||
| response.headers.get('X-Snapshot-Events-Cursor') || null; |
Contributor
There was a problem hiding this comment.
load fabricates metadata instead of failing loudly. (a) save writes metadata.eventsCursor ?? '' (L51), so a server that drops the header is indistinguishable from a snapshot taken at cursor-null — i.e. "replay from the beginning", the silent-wrong-answer direction. (b) createdAt falls back to new Date(), inventing a timestamp. Unlike local/postgres, this path never runs SnapshotMetadataSchema.parse. Treat a missing cursor header as malformed (throw, or return null so the caller cold-starts) and parse through the schema.
| }; | ||
| }, | ||
|
|
||
| async delete(runId: string): Promise<void> { |
Contributor
There was a problem hiding this comment.
Delete throws on 404 while local (force: true) and postgres (plain DELETE) are idempotent. The interface JSDoc says delete runs at terminal state — exactly the path most likely to retry, run twice, or run for a run that never snapshotted. Add 404 to the success set, or document non-idempotence and make #3251 tolerate the throw.
| // Diagnostic: actual on-the-wire snapshot bytes and the HTTP-PUT | ||
| // cost, grep-able by runId alongside @workflow/core's QUICKJS_VM | ||
| // diagnostics. | ||
| console.debug('[Workflow] WORLD_SNAPSHOT_DIAG', { |
Contributor
There was a problem hiding this comment.
Ungated console.debug on a path that runs on every suspension/resume (also at L139 and the load path). Route through the package's existing httpLog/debug gating.
| * data so that on restore, only events created after the snapshot need | ||
| * to be fetched. | ||
| */ | ||
| snapshots: { |
Contributor
There was a problem hiding this comment.
Breaking change labeled minor. This is a required member of Storage, and the changeset itself says community worlds must add it — that's a major for @workflow/world. worlds-manifest.json lists 13 out-of-repo implementations (@workflow-worlds/*, @fantasticfour/world-*, @platformatic/world, workflow-world-jazz); all fail to typecheck, and none are in CI, so this is invisible until they upgrade. In-repo mocks dodge it via as unknown as World, which is why CI is green.
Pick deliberately: make it snapshots?: and have #3251 feature-detect (keeps this a true minor, and a world that can't store blobs should be allowed to simply not — #3251 already needs a no-snapshot cold-start path), or keep it required and bump major. I'd favor optional. The bump type also determines the semver step on any stable backport, so the mislabel isn't cosmetic.
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
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.
Note
Supersedes #3050, which GitHub auto-marked as merged (and auto-deleted the head branch) when a restacking mistake briefly force-pushed the head branch to the same commit as its base. Same content, freshly rebased on the stack.
Summary
PR 3 of the QuickJS VM roadmap: the
world.snapshotsstorage interface and its implementations, ported from the original snapshot-runtime branch (#1300). Inert until PR 4 — nothing in the runtime calls these APIs yet.What's included
@workflow/world:Storage.snapshotsinterface —save(runId, data, metadata)/load(runId)/delete(runId)— plusSnapshotMetadata(eventsCursor,createdAt). The cursor is storage-layer metadata stored alongside the opaque snapshot bytes so a restore only fetches events recorded after the snapshot.@workflow/world-local: filesystem storage under{basedir}/snapshots/({runId}.bin+{runId}.jsonsidecar), with round-trip unit tests.@workflow/world-postgres:workflow_snapshotstable (migration0016, renumbered from the original branch to follow main's0012–0015).@workflow/world-vercel: client for the workflow-server snapshot endpoints (PUT/GET/DELETE /v2/runs/:runId/snapshot, binary body + metadata headers — endpoints are live in workflow-server since vercel/workflow-server#339). Includes:undici.request()(notfetch()) PUT path so Buffer bodies survive RetryAgent retries (Retry Handler Fails When Request Has a Body nodejs/undici#3288),makeRequest, per the repo's trace-propagation rule), covered by newtrace-propagation.test.tscases.Notes
@workflow/core's responsibility (PR 4's save pipeline); worlds must not add their own compression (ciphertext doesn't compress).Storageis a breaking change for community World implementations — appropriate for the v5 beta line; called out in the changeset.WORLD_SNAPSHOT_DIAGwarn logging from the original branch was demoted toconsole.debug.