Repository navigation
Add experimental world.experimental_snapshots storage interface (local, postgres, vercel) - #3250
Conversation
🦋 Changeset detectedLatest commit: a027985 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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 |
🧪 E2E Test Results✅ All tests passed 🛠 Infra Events (absorbed by the harness)Platform anomalies the e2e harness detected and worked around (e.g. a run the queue never picked up, replaced by a fresh run). Clustered timestamps indicate a backend blip; a steady drip indicates a platform issue worth escalating.
E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 🌐 Cross-language Conformance
✅ dynamic-runs
✅ vercel-http-transport
✅ vercel-multi-region
✅ vercel-ws-transport
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
a4e1f58 to
c935825
Compare
📊 Workflow Benchmarkscommit Backend:
Streams
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 249094ms → this run 191577ms (Δ -57517ms, -23%) 📈 CRTT drill-down vs main (RTT distributions & profiles)RTT over stream progress (avg per tenth of stream, bars scaled min→max): RTT by chunk size (avg per log size bin, ~160B → ~12KB serialized, bars scaled min→max): Delivery jitter over stream progress (avg positive CDV per tenth of stream, bars scaled min→max): ℹ️ Metric definitions & methodologyStreams: first-chunk RTT (the stream-open path, before any buffering/backpressure), CRTT percentiles, and worst delivery stall (CDV max). Cells are medians across iterations; per-run values in the artifacts. No 🔴/🟢 marks until targets attach. The collapsed STSO distribution section above buckets every step gap, split inline (same warm process — pure framework overhead) vs queue-hop (fresh process — dispatch, reinit, replay). The collapsed CRTT drill-down: per-variant RTT histograms (fixed log bins, 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) · Fan-out TTFS: fan-out time to first step (in-deployment start() → first of the parallel step bodies to complete) · Fan-out TTLS: fan-out time to last step (in-deployment start() → last of the parallel step bodies to complete, i.e. when the Promise.all resolves) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · CRTT: chunk round-trip time (per-chunk write → read latency, one clock domain: deployment → stream backend → same deployment) · CDV: chunk delay variation / delivery jitter (inter-arrival gap minus inter-write gap per seq-adjacent pair; skew-free; the row is each run's MAX positive value, so one stall moves it) 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 · Promise.all(100 steps): 100 trivial no-op steps started together in a single Promise.all; Fan-out TTFS is the first of them to complete and Fan-out TTLS the last, both from the in-deployment clientStart, so their gap is the spread the runtime adds across the fan-out · paced control (100/s, 60B): the control: 300 tiny (~60B) deltas metronome-paced at 100/s — zero workload structure, so it reads the transport floor and flush cadence, and disambiguates transport-wide vs workload-specific when a replay row moves · size sweep (100/s, 160B-12KB): same pacing as the control with deltas padded in rotation across seven log-spaced sizes (~160B–12KB) — rotation decouples size from stream position, so it isolates whether chunk size causes latency · replay gateway-gpt-5.4-nano-2000t (1x): raw provider SSE cadence captured at the AI gateway boundary (gpt-5.4-nano, the most popular gateway model; per-token deltas p50 208B = the modal production chunk size), replayed exactly as measured — the typical customer's workload; its CDV is the typical customer's real delivery jitter · replay eve-gpt-5.6-sol-2000t (1x): a captured eve turn (gpt-5.6-sol, the most-used demanding eve model; ~2000 output tokens = production p50 turn length) replayed exactly as measured — eve's envelope protocol re-ships the cumulative message so sizes ramp 142B→13KB; the demanding outlier tenant's reality · replay eve-gpt-5.6-sol-2000t (2x): the same eve capture at 2x — the headroom/stress row; real fast-tier models emit the same chunk sizes at proportionally higher rate, so time compression is a faithful speed model · first chunk (pooled): every run's seq-0 RTT pooled across all stream scenarios — the first chunk precedes any workload differentiation, so pooling samples one shared stream-open path with exact percentiles Replay cadences (semantic sha256) — eve-gpt-5.6-sol-2000t 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 All timestamps are deployment-side; runs are triggered in-deployment, so the CI runner and api.vercel.com sit outside every measured window. TTFS = Cold starts stay in the numbers (real bursty-workload latency, inflates P75+); Best is the warm floor. |
pranaygp
left a comment
pranaygp
left a comment
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([ |
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.
There was a problem hiding this comment.
Fixed in 374dbaa by restructuring the storage to a single self-describing blob: new encodeSnapshotEnvelope/decodeSnapshotEnvelope helpers in @workflow/world pack the metadata and bytes into ONE file ({runId}.snapshot), written with the existing atomic temp+rename. The torn window is structurally gone — there is no second file to pair wrongly. A corrupt/truncated envelope decodes to null (clean miss → full replay), never torn state. Test added: corrupt-envelope-as-miss + single-file assertion.
| const data = new Uint8Array(buffer); | ||
|
|
||
| const eventsCursor = | ||
| response.headers.get('X-Snapshot-Events-Cursor') || null; |
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.
There was a problem hiding this comment.
Fixed in 374dbaa: the envelope (see the torn-save thread) is now the GET/PUT body, and load decodes metadata exclusively from it — schema-validated via SnapshotMetadataSchema inside decodeSnapshotEnvelope. A body that doesn't decode returns null (cold start), even when plausible-looking headers are present; nothing is ever fabricated from headers or wall time. The X-Snapshot-* headers are still sent on save as observability-only denormalized copies. Tests added: full-metadata round-trip with NO headers, and undecodable-body-as-miss with headers present.
| }; | ||
| }, | ||
|
|
||
| async delete(runId: string): Promise<void> { |
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.
There was a problem hiding this comment.
Fixed in 374dbaa: DELETE now treats 404 as success, matching local's force: true and postgres's plain DELETE, and the interface JSDoc now states the idempotence requirement explicitly. Tests added for both 404-is-success and non-retryable-error-still-throws (403 — the shared RetryAgent retries 5xx, which is orthogonal).
| // 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', { |
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.
There was a problem hiding this comment.
Fixed in 374dbaa: all three WORLD_SNAPSHOT_DIAG sites route through a helper gated on the package's existing HTTP_DEBUG_ENABLED flag (same gate as httpLog).
| * data so that on restore, only events created after the snapshot need | ||
| * to be fetched. | ||
| */ | ||
| snapshots: { |
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.
There was a problem hiding this comment.
Fixed in 374dbaa — went with your preferred option: snapshots?: is now optional on Storage, so this is a true minor and the 13 out-of-repo worlds keep typechecking. #3251's entrypoint feature-detects (world.snapshots absent ⇒ threshold forced to 0 ⇒ pure full replay, always correct). The changeset text was updated to say community worlds are unaffected. The interface JSDoc also now spells out the two new contract points this review surfaced: metadata must round-trip losslessly AND atomically with the bytes it describes (the envelope helpers make a plain blob store satisfy that with one write), and delete must be idempotent.
Sim WorldSimulated world deterministic testing for races. Traces 🟠 world-sim scenario book — 1 fail of 42 total
Full trace: |
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Review from a DynamoDB read-throttling investigation on workflow-server. Notes prefixed AI (found independently) or AI+Human (a colleague pointed me at the area).
One correction to my own first read: I initially assumed the X-Snapshot-* headers were the metadata contract and started testing them as correctness-critical. The doc comment in snapshots.ts sets me straight — the body is a self-describing envelope and "loads decode the envelope and never trust headers". That's the better design, and the reasoning about an invented null cursor meaning "replay from the beginning" is exactly right. My notes below reflect the envelope model.
| * an invented null cursor means "replay from the beginning"). | ||
| * | ||
| * Snapshot endpoints use raw binary transfer: | ||
| * - PUT /v2/runs/:runId/snapshot — envelope body |
There was a problem hiding this comment.
AI+Human — these endpoints had no test coverage until now; I added some and they pass ✅
Worth flagging that the workflow-server endpoints this client targets shipped in vercel/workflow-server#339 back in March 2026 and have had zero integration tests and zero production traffic (0 requests over a 7-day window, against ~236M event-list requests). This PR is the first thing that will exercise them for real.
I wrote an integration suite against them (LocalStack S3 + the real Next.js server). All 7 pass, so the wire contract is sound:
- Byte-exact round-trip using deliberately non-UTF8 bytes (
0x00 0xff 0x1f 0x8b 0x80 0xfe). This is the one that matters for the envelope model — any accidental string coercion in transport would corrupt the blob silently. It survives. - 4 MB payload (roughly the measured compressed heap size) round-trips byte-identical.
- Miss returns a clean 404, which is what fallback-to-full-replay depends on.
- DELETE is idempotent — second call 200, subsequent GET 404. Matches the "404 is success" contract documented here.
- 64 MB cap rejects with a clean
400(Snapshot size 67109888 exceeds maximum 67108864) and leaves no partial object behind — a follow-up GET is 404. So the degradation you want on oversize is available. - Cross-tenant read rejected.
- The
X-Snapshot-*headers do round-trip server-side, so they're usable for the observability purpose described here.
Happy to open that as a PR against workflow-server so this lands on tested ground — just say the word.
One suggestion from the cap result: catch save failures so an oversized heap skips the snapshot rather than failing the suspension, and consider a client-side size check with a metric. The runs that snapshot are by definition the long ones, and heap grows with retained state.
There was a problem hiding this comment.
AI: Thanks for running these against the real endpoints. The byte-exact non-UTF8 round-trip and clean 404 on miss are the two properties the envelope model depends on.
On the save-failure suggestion: the core pipeline in #3251 already treats save as best-effort. Compress → encrypt → save runs under waitUntil with a warn-and-continue error handler, so a failed save never fails the suspension; the run keeps going on full replay and the next qualifying suspension tries again. #3251 also skips any heap over 32 MB plaintext before uploading, and latches that run so later suspensions skip the capture too.
I've added the client-side check you suggested in 12817aa. save now refuses an envelope over the backend's 64 MiB limit before sending anything: it logs a too_large diagnostic and throws a WorkflowWorldError, which the core swallows like any other save failure. A test confirms nothing goes over the wire. For the metric, the WORLD_SNAPSHOT_DIAG line (debug-gated) and core's snapshot_saved/size-skip warn are the current hooks. A proper counter can come once snapshots have real traffic.
| * | ||
| * Snapshot endpoints use raw binary transfer: | ||
| * - PUT /v2/runs/:runId/snapshot — envelope body | ||
| * - GET /v2/runs/:runId/snapshot — envelope response |
There was a problem hiding this comment.
AI — two workflow-server gaps this client consumes but can't fix
Filing here for visibility; happy to move them to workflow-server issues if you'd rather.
1. The snapshot object's S3 TTL is hardcoded to 90 days, not derived from the tenant's data policy:
Tagging: S3_TTL_TAGS[90],Every other S3 object in the run lifecycle gets dataPolicy.storageRetentionDays via _batchSetS3TTLs. And the snapshot key is in neither s3KeysFromRefs nor streamS3Refs, so finalizeRunStorage never re-tags it — that write-time 90-day tag is the only lifecycle it ever gets.
To be fair, a write-time tag is better than the comparable hook path, which has no create-time TTL at all and leaks orphans indefinitely. But a customer on 30-day retention would have snapshots outliving their own data by 60 days, which looks unintended.
2. Snapshot bytes are metered but excluded from the storage-duration bill. saveSnapshot/loadSnapshot emit WORKFLOW_STORAGE_WRITE/_READ usage facts, but snapshots never go through trackBlobStorageBytes, so they don't land in run.blobStorageBytes — which means finalizeRunStorage's billStorage(..., blobStorageBytes + streamStorageBytes, ...) excludes them. At ~4 MB per snapshot on long-running runs that's a deliberate-looking asymmetry with blobs and streams; just want to confirm it's intentional rather than an oversight.
There was a problem hiding this comment.
AI: Both of these are backend-side, so I haven't changed anything here, but they're real follow-ups and worth filing against the backend:
- Retention: snapshot objects should follow the tenant's data-retention policy rather than a fixed TTL, and should be covered by run finalization like other run-scoped objects. The SDK deletes snapshots on terminal states it observes, but a backend TTL tied to retention is the backstop for runs that end without any invocation seeing it.
- Billing: whether snapshot bytes count toward storage duration is a product decision, not something the client can settle. Agreed the asymmetry with blobs and streams should be deliberate either way.
Nothing on the client depends on how these are resolved: the wire contract (envelope body, idempotent DELETE, 404 miss) stays the same.
|
AI: I synced this PR with the latest
Earlier review commits (c935825, 374dbaa, …) remain reachable through the PR timeline. |
About these numbersSizes are gzip; parentheses show the change against
|
|
AI: Renamed the interface to |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
LGTM pending changeset update
Squashed content of #3250, synced with main. The Postgres migration is renumbered to 0024 to follow main's migrations. Co-Authored-By: Nathan Rajlich <71256+TooTallNate@users.noreply.github.com>
…to workflow-server Co-Authored-By: Nathan Rajlich <71256+TooTallNate@users.noreply.github.com>
…hot API experimental World implementations don't need to provide snapshot storage; the name and @experimental JSDoc on the interface and the @workflow/world snapshot exports make that explicit. The changeset now calls out the new Postgres table. Co-Authored-By: Nathan Rajlich <71256+TooTallNate@users.noreply.github.com>
Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Nathan Rajlich <n@n8.io> Co-Authored-By: Nathan Rajlich <71256+TooTallNate@users.noreply.github.com>
Co-Authored-By: Nathan Rajlich <71256+TooTallNate@users.noreply.github.com>
|
No backport to This commit adds a brand-new (experimental) To override, re-run the Backport to stable workflow manually via |
Note
Supersedes #3050. Stacked: #3251 builds on this PR.
Summary
The optional, experimental
experimental_snapshotsstorage interface onWorldand its implementations. Nothing calls it until #3251 (QuickJS threshold-based VM snapshotting).What's included
@workflow/world:Storage.experimental_snapshots?withsave(runId, data, metadata)/load(runId)/delete(runId), plusSnapshotMetadataand theencodeSnapshotEnvelope/decodeSnapshotEnvelopehelpers, which pack metadata and bytes into one blob so a plain blob store can write both atomically. All of it is marked@experimental. World implementations don't need to provide it: without it the runtime always does a full event replay.@workflow/world-local: one envelope file per run under{basedir}/snapshots/, withrunIdvalidated before any path is built.@workflow/world-postgres: a newworkflow_snapshotstable (migration0025_add_snapshots_table) storing the envelope.@workflow/world-vercel: client for the backend's snapshot endpoints (PUT/GET/DELETE /v2/runs/:runId/snapshot, envelope as the body). It includes:undici.request()PUT path, so Buffer bodies survive RetryAgent retries (Retry Handler Fails When Request Has a Body nodejs/undici#3288);makeRequest;Notes
@workflow/core(QuickJS engine: threshold-based VM-memory snapshotting (WORKFLOW_SNAPSHOT_THRESHOLD) #3251); worlds must not compress them (ciphertext doesn't compress).