Attributes MVP (experimental and write-only)#2088
Conversation
Outlines the bare-MVP, write-only attributes design that defers the `attr_set` event type (and the associated SPEC_VERSION_CURRENT bump) to the full 5.0.0 feature. Forward-compatible SDK surface and wire format. `experimentalSetAttributes` is optional on the World interface so third-party worlds keep working. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
🦋 Changeset detectedLatest commit: 4aa2bb4 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 |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests💻 Local Development (21 failed)nextjs-webpack-canary (7 failed):
nextjs-webpack-stable-lazy-discovery-disabled (7 failed):
nextjs-webpack-stable-lazy-discovery-enabled (7 failed):
Details by Category✅ ▲ Vercel Production
❌ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Implements the V5 Workflow Attributes MVP per the changelog plan:
- @workflow/world: shared validation + apply helpers; optional
experimentalSetAttributes on Storage.runs; attributes field on
WorkflowRunBaseSchema. Optional so third-party worlds keep working.
- @workflow/core: setAttributes() helper. Detects workflow VM vs step
context, normalizes undefined→null, validates client-side, dispatches
via an internal "use step" function. Feature-detects the world method
and no-ops with a one-time warning if missing.
- @workflow/world-local: file-backed impl with a per-run async mutex
so concurrent writes do not lose updates within a process. Threads
attributes through the run lifecycle event reconstructions so they
survive subsequent run_started/_completed/_failed/_cancelled writes.
- @workflow/world-postgres: jsonb column with SQL-side atomic merge
(jsonb_set / `-`); 0013 migration.
- @workflow/world-vercel: HTTP wrapper posting the documented
{ changes: [...] } body to /v2/runs/:runId/attributes.
Tests: 18 validation unit + 10 SDK unit + 10 world-local integration +
3 world-postgres integration. End-to-end coverage in the workbench is
deferred until the paired workflow-server endpoint is deployed to a
preview.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
VaguelySerious
changed the title
The previous design used a 'use step' indirection inside @workflow/core so setAttributes could be called from both workflow and step bodies via a single SDK surface. That broke nextjs-webpack Local Dev: the deferred- entries discoverer in webpack dev mode walks transitive imports from 'use step' files, and putting a step file inside @workflow/core/dist pulled host-side world adapters and @vercel/queue into the step-discovery graph. Webpack's regex-based import extractor then blew the call stack with "RangeError: Maximum call stack size exceeded at RegExpStringIterator.next" on tarball-installed deployments. runtime/start.ts and runtime/run.ts get away with the same directive because they're never reachable from packages/core/src/workflow/index.ts (the VM bundle entry); ours was. A host-side bridge comparable to sleep would have fixed it but is substantial wiring for a feature whose end state (event-sourced attr_set) replaces the bridge mechanism entirely. Pragmatic MVP path: restrict to step body and let users wrap in a step explicitly. Full 5.0.0 lifts the restriction via attr_set events through the workflow controller; SDK signature is stable across the cutover. - Workflow-VM-side setAttributes throws FatalError with wrap-in-step instructions - Step-side setAttributes works (validates, dispatches, world-detects) - set-attributes-shared.ts is now pure validation; no 'use step', no world imports - Test updated to assert the FatalError on workflow-body calls - Changelog MDX updated with the new scope + a "Why workflow-body dispatch is deferred" implementation note Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
… setAttributes is "supported via the host-side bridge in `set-attributes.ts`, which calls into an actual step via `registerStepFunction`" — no such bridge or registerStepFunction usage exists. This commit fixes the issue reported at packages/core/src/set-attributes-shared.ts:25 **Bug:** The comment on lines 24-26 of `packages/core/src/set-attributes-shared.ts` describes an intermediate design approach that was abandoned before the final implementation. It states: "workflow-body use is supported only via the host-side bridge in `set-attributes.ts`, which calls into an actual step via `registerStepFunction`." In the actual final implementation: 1. `packages/core/src/workflow/set-attributes.ts` (the workflow-VM-side export) unconditionally throws `FatalError` — there is no bridge support at all. 2. `packages/core/src/set-attributes.ts` (the host-side export) explicitly checks for workflow-body context and throws `FatalError` with a message telling users to wrap the call in a `'use step'` function. 3. A grep for `registerStepFunction` in combination with `setAttributes` returns zero results — no such wiring exists. The comment is misleading to any developer reading the codebase: it implies workflow-body use works via a bridge mechanism, when in fact it throws a fatal error. **Fix:** Updated lines 24-26 to accurately describe the actual behavior: "workflow-body use throws FatalError — users must wrap the call in their own `'use step'` function." This aligns with the implementation in both `set-attributes.ts` and `workflow/set-attributes.ts`, and with the JSDoc on `setAttributes` which explicitly documents the step-body-only restriction and the workaround pattern. Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com> Co-authored-by: VaguelySerious <mittgfu@gmail.com>
For local e2e validation against the workflow-server attributes-mvp preview deployment. Do not merge — this constant must be empty on main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n step Workflow-body `setAttributes` calls now dispatch through an internal `__builtin_set_attributes` step rather than throwing FatalError. The workflow-VM helper validates input and then invokes a host-side useStep dispatcher pre-bound under WORKFLOW_SET_ATTRIBUTES; the step body forwards to the same world adapter call the step-body path uses. Putting the 'use step' directive inside `packages/workflow/src/internal/builtins.ts` (next to the existing `__builtin_response_*` builtins) instead of `@workflow/core/dist/` avoids the deferred-entry discoverer hazard that motivated the prior MVP-only step-body restriction. - Add `WORKFLOW_SET_ATTRIBUTES` symbol + workflow.ts wiring - New `step-set-attributes.ts` host helper (`applySetAttributesChanges`) shared between step-body and workflow-body paths - `__builtin_set_attributes` builtin step - Refactor workflow-side `setAttributes` to dispatch via the bridge - Update changelog MDX + add tests (16 unit, 2 e2e) - e2e workflows `setAttributesFromStepWorkflow` and `setAttributesFromWorkflowBodyWorkflow` cover both paths Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The comment described the prior FatalError-on-workflow-body workaround; update it to reflect the current bridge-via-builtins design. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…verer
`__builtin_set_attributes` dynamically imported
`@workflow/core/_step-set-attributes` as a literal string. The Next.js
deferred-entries discoverer in `@workflow/next/builder-deferred.ts`
matches `import('...')` regex-style and walks the resolved file's
transitive imports — which reach the world adapter and `@vercel/queue`,
triggering `RangeError: Maximum call stack size exceeded` inside
`RegExpStringIterator.next` on tarball-installed nextjs-webpack
builds. The build never completes, so dev-mode e2e tests time out
across the board.
Assemble the specifier at runtime (same trick `get-world-lazy.ts`
uses for `./world.js`) so the discoverer doesn't see a literal target.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…idge The runtime-built specifier broke step bundle resolution across every framework — `Cannot find package '@workflow/core' imported from .../node_modules/.nitro/workflow/steps.mjs`. Bundlers can't statically resolve a concatenated string, so the dependency never lands in the step bundle and Node's loader fails at runtime. Reverts fbb0c59. The original webpack-dev discoverer-overflow it tried to fix only affected 3 jobs; this regression took down 30+ jobs across all frameworks. The discoverer issue needs a different fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ngle dispatch path
Drops step-body support for the MVP. The architecture becomes:
- workflow VM `setAttributes` validates inline and dispatches via the
standard `globalThis[WORKFLOW_USE_STEP]('__builtin_set_attributes')`
mechanism — same as every other step call from a workflow body
- host-side `setAttributes` is a stub that throws FatalError telling
callers to use a workflow body
- `__builtin_set_attributes` step body reads world + run id directly
from `globalThis` symbols populated by the runtime, with no imports
from `@workflow/core`
This deletes the bridge plumbing the previous design needed:
- `WORKFLOW_SET_ATTRIBUTES` global symbol + the workflow.ts pre-bind
- `packages/core/src/set-attributes-shared.ts` (normalize helper)
- `packages/core/src/step-set-attributes.ts` (host-side helper)
- the `@workflow/core/_step-set-attributes` package export and the
dynamic import that pulled it in from the step bundle
Side benefit: the Next.js deferred-entries discoverer can no longer
walk from `__builtin_set_attributes` into the world adapter / queue
chain that broke webpack-dev builds in the previous shape, because the
step body holds zero @workflow/core imports.
Workbench example and e2e tests reduced to a single
`setAttributesWorkflow` covering workflow-body dispatch. World-side
implementations are unchanged; step-body support can be added later
without touching the workflow-body contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| const WORKFLOW_SERVER_URL_OVERRIDE = | ||
| 'https://workflow-server-git-peter-attributes-mvp.vercel.sh'; |
There was a problem hiding this comment.
WORKFLOW_SERVER_URL_OVERRIDE is set to a preview branch URL instead of an empty string, which would route all production @workflow/world-vercel clients to a preview-branch workflow-server if merged to main.
| */ | ||
| const WORKFLOW_SERVER_URL_OVERRIDE = ''; | ||
| const WORKFLOW_SERVER_URL_OVERRIDE = | ||
| 'https://workflow-server-git-peter-attributes-mvp.vercel.sh'; |
There was a problem hiding this comment.
Must revert before merge. WORKFLOW_SERVER_URL_OVERRIDE is set to a preview-branch URL (workflow-server-git-peter-attributes-mvp.vercel.sh) instead of ''. The comment block right above this assignment says this constant defaults to main and is only rewritten by external CI for branch testing — committing a non-empty value here routes every @workflow/world-vercel production client (Next.js, Hono, etc.) at the preview-branch workflow-server once this PR merges to main.
| // counted as +1 here, which makes the cap check slightly conservative. | ||
| // For the MVP cap of 64 this is acceptable; the server's authoritative | ||
| // check uses the real post-merge size. | ||
| if (change.value !== null) netAdds += 1; |
There was a problem hiding this comment.
The cap check counts every value !== null change as a fresh add, even when the key already exists on the run. Once a run hits 64 attributes, updating a single existing key fails: existing=64, netAdds=1, 64 + 1 > 64 — even though the post-merge size is still 64.
The inline comment calls this "slightly conservative" and acceptable for the MVP, but in practice it prevents legitimate updates and surfaces as an "exceeds limit 64 (existing 64 + incoming 1)" error that a user has no way to interpret. Mitigation requires knowing which incoming keys already exist, which is fine for the world-side check (it has the snapshot) but means the client-side validation in workflow/set-attributes.ts (which passes no existingCount) is doing nothing useful here.
Suggest: have world-side validators subtract changes.filter(c => c.value !== null && existing[c.key] !== undefined).length from netAdds, and accept that client-side validation only catches per-batch limits.
| | undefined; | ||
| if (typeof world?.runs?.experimentalSetAttributes !== 'function') { | ||
| // World adapter doesn't implement attributes yet — silently no-op. | ||
| // The VM-side validation already ran, so input was well-formed. |
There was a problem hiding this comment.
Silent no-op contradicts the documented behavior. The MVP changelog (docs/content/docs/v5/changelog/attributes-mvp.mdx) and the JSDoc on Storage.runs.experimentalSetAttributes (packages/world/src/interfaces.ts:177-180) both state: "the SDK detects absence and no-ops setAttributes with a warning so that third-party / community worlds continue to function without adopting the experimental API."
This code returns silently with no log/warning. A user calling setAttributes() against a community world that hasn't adopted the API will see a successful await and have no signal whatsoever that their attributes weren't persisted. That's the exact failure mode the documented warning is supposed to prevent.
At minimum, emit runtimeLogger.warn(...) (or a process-lifetime once-flag if you're worried about noise) before return. Right now the docs/code drift is also itself a maintainability concern — either implement the warning or strike the claim from the docs.
|
|
||
| if (!updated) { | ||
| throw new WorkflowRunNotFoundError(runId); | ||
| } |
There was a problem hiding this comment.
The per-run cap is not enforced atomically — two concurrent writers can both pass validation and produce a row over the 64-attribute limit.
Walkthrough (cap=64):
- Run currently has 60 attributes.
- Writer A reads
existing.attributes→ 60 keys. - Writer B reads
existing.attributes→ 60 keys. - A validates with
existingCount=60adding 3 new → 63 ≤ 64 ✓. - B validates with
existingCount=60adding 3 new (different keys) → 63 ≤ 64 ✓. - A's
UPDATEruns → row has 63 attributes. - B's
UPDATEruns against the now-mutated row → ends up with 66 attributes.
The inline comment acknowledges this as "last-write-wins by arrival" but conflates write order with the cap invariant — LWW only governs which value of a given key survives; it does not stop concurrent disjoint-key inserts from blowing past the size cap. The comment in attributes.ts ("the server's authoritative check uses the real post-merge size") is also misleading for the Postgres world, since this read-then-update is the only authoritative path here.
Options: (a) wrap in a serializable transaction, (b) compute & assert the post-merge length inside the same UPDATE via a SQL CASE WHEN jsonb_object_keys(...) > 64 THEN error (Postgres doesn't make this elegant but RAISE inside a function works), or (c) accept the loose cap and document that the limit is best-effort under concurrent writers.
| completedAt: undefined, | ||
| startedAt: currentRun.startedAt ?? now, | ||
| updatedAt: now, | ||
| attributes: currentRun.attributes, |
There was a problem hiding this comment.
Run-lifecycle event writes share the run file with experimentalSetAttributes but coordinate via no shared mutex — attribute writes can be silently lost.
The new withRunAttributeLock in runs-storage.ts only serializes calls to experimentalSetAttributes against each other. The run-lifecycle handlers (run_started, run_completed, run_failed, run_cancelled) here all use the read-then-write pattern: they read currentRun near the top of createImpl (line ~219), then build a fresh run object preserving attributes: currentRun.attributes and writeJSON({ overwrite: true }) it back. There's no lock around that span.
Race example: a __builtin_set_attributes step runs while run_completed is being persisted for the same run.
- events-storage reads
currentRunwithattributes = { phase: 'init' }. experimentalSetAttributeslands and writes{ phase: 'init', orderId: 'ord_123' }.- events-storage continues, constructs the completed run with
attributes: currentRun.attributes(= the stale{ phase: 'init' }), and overwrites the file. - The
orderIdwrite is silently lost.
I don't think the in-process mutex can fix this cleanly — events-storage and runs-storage are independent write paths against the same file. The fix is probably to route attribute writes through the same per-run lock the lifecycle writer takes, or (cleaner) to re-read the run inside the lock and merge before writing on lifecycle events. Worth a comment at minimum if you're choosing to accept the race for the MVP.
| 'It must be called from within a workflow body (`use workflow`).' | ||
| ); | ||
| } | ||
| await useStep('__builtin_set_attributes')(changes); |
There was a problem hiding this comment.
Please verify the bare-name step lookup for __builtin_set_attributes actually resolves under the production SWC + step-handler pipeline.
The workflow VM dispatches with the bare name '__builtin_set_attributes', which becomes the stepName on the step_created event. On the step worker side, packages/core/src/runtime/step-handler.ts:198 calls getStepFunction(stepName) to resolve it.
getStepFunction (packages/core/src/private.ts:103-122) tries three things in order:
registeredSteps.get(stepName)— but the SWC plugin spec (spec.md:115) registers steps with full IDs likestep//<modulePath>//<fnName>, so a bare-name lookup misses.getStepIdAliasCandidates(stepName)— returns[]when the input isn't already astep//x//ytriple.getBuiltinResponseStepAlias(stepName)— this is the only fallback that handles bare names, and it explicitly hard-codesBUILTIN_RESPONSE_STEP_NAMES = { __builtin_response_array_buffer, __builtin_response_json, __builtin_response_text }atprivate.ts:30-34.__builtin_set_attributesis not in that set, so the fallback returns undefined.
That private.ts file is not touched by this PR. The way __builtin_response_* work today is precisely that they're in the allowlist; __builtin_set_attributes needs the same treatment (or a generalization of the alias to all step//*//<bareName> matches).
The new e2e (packages/core/e2e/e2e.test.ts) does seem to pass in CI, which is what makes me less than fully confident — but I can't see a code path that makes the bare-name lookup succeed. Worth running the new e2e explicitly with getStepFunction instrumentation, or just adding __builtin_set_attributes to BUILTIN_RESPONSE_STEP_NAMES (and probably renaming that set) to be safe.
| * } | ||
| * ``` | ||
| */ | ||
| export async function setAttributes( |
There was a problem hiding this comment.
Naming nit: prefer experimental_setAttributes for the public export.
The repo doesn't yet have an established experimental_ / unstable_ convention, but adopting one here would make the "this WILL change" signal much louder at the call site — await setAttributes({...}) reads as production-ready, await experimental_setAttributes({...}) does not. This is the Next.js / React pattern (unstable_*, experimental_*) and it's a single codemod to remove when V1 stabilizes the API — which doubles as a forcing function for every consumer to re-review the new contract on the rename.
Suggest applying the same convention to the world-interface method too: experimental_setAttributes? rather than experimentalSetAttributes? in packages/world/src/interfaces.ts. Consistency across the surface, and the underscore separator makes the experimental marker scan visually rather than blending into a camelCase identifier.
| */ | ||
| export async function setAttributes( | ||
| _attrs: Record<string, string | undefined> | ||
| ): Promise<void> { |
There was a problem hiding this comment.
Is the workflow-body-only restriction load-bearing, or scope?
Step-body support looks like ~10 lines: the host stub would do exactly what __builtin_set_attributes already does — read the runId from Symbol.for('WORKFLOW_STEP_CONTEXT_STORAGE'), read the world from Symbol.for('@workflow/world//cache'), validate, dispatch directly. Both symbols are already populated when a step body runs (the bridge step proves it).
const ctx = globalThis[Symbol.for('WORKFLOW_STEP_CONTEXT_STORAGE')]?.getStore?.();
const runId = ctx?.workflowMetadata?.workflowRunId;
if (!runId) throw new FatalError('must be called from workflow or step body');
const world = globalThis[Symbol.for('@workflow/world//cache')];
await world?.runs?.experimentalSetAttributes?.(runId, normalizedChanges);Plain host code outside any run genuinely can't be supported with this signature (no runId to infer) — that part isn't arbitrary, you'd need a separate experimental_setRunAttributes(runId, {...}). But step body is just a scope decision.
Trade-offs of allowing step body:
What you'd gain:
- Users writing a step that already has the data don't have to bubble values back to the workflow body to set an attribute.
- The natural ergonomic model —
setAttributesworks "anywhere inside a run."
What you'd lose:
- The "single dispatch path" simplicity the PR description cites.
- Event-log visibility of the dispatch: workflow-body calls produce a
step_created/step_completedpair via the__builtin_set_attributesbridge; step-body calls would write directly with no extra events. Replay determinism is unaffected (step bodies aren't re-executed during replay anyway).
Forward-compat with #1933 is fine either way — the planned attr_set event's writer: { type: 'step', stepId, attempt } discriminator already accounts for step writers.
Not asking you to add it in this PR — but if the restriction is just scope-cut rather than something the architecture is leaning on, please mention that in the changelog so it doesn't read as a hard architectural constraint that has to stay until V1.
`extractBundleSourceFiles` used `matchAll` with `/...[A-Za-z0-9+/=]+.../g` to pull inline base64 source maps out of generated bundles. V8's irregexp uses recursion for greedy character-class quantifiers, and on bundles with multi-MB inline sourcemaps the engine exhausts the stack mid-match with `RangeError: Maximum call stack size exceeded at RegExpStringIterator.next`. That broke nextjs-webpack-dev e2e jobs on this branch after main enabled inline sourcemaps across all workspace packages (#1799). Switch to a literal-prefix scan with a manual base64 alphabet loop — linear time, no recursion. The character-code check inlines what the regex was doing (`[A-Za-z0-9+/=]`), so behaviour is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… post-merge cap - Rename the public SDK export from `setAttributes` → `experimental_setAttributes` to signal the unstable surface at every call site (workbench, docs, e2e + unit tests updated). Internal step name stays `__builtin_set_attributes`. - `validateAttributeChanges`: replace `existingCount?: number` with `existingKeys?: Iterable<string>`. With keys, the cap check uses real net adds/deletes (an update to an already-present key is zero net); without keys it falls back to the conservative "every upsert is +1" shape. Fixes the off-by-design rejection of single-key updates at the cap boundary. - `world-local`: rename `withRunAttributeLock` → `withRunFileLock`, export it, and acquire it from the events-storage run-lifecycle branches (`run_started`/`run_completed`/`run_failed`/`run_cancelled`). Each lifecycle write re-reads the run JSON inside the lock so an attribute write that landed between the pre-validation read and the write is no longer silently overwritten. - `world-postgres`: atomic per-run cap. The cap check now lives in the same `UPDATE` statement as the merge (`WHERE (SELECT COUNT(*) FROM jsonb_object_keys(merged_expr)) <= ATTRIBUTE_MAX_PER_RUN`), so two concurrent writers adding disjoint keys at the cap boundary can no longer both succeed and push the row past 64. A separate re-read on rejection disambiguates "run not found" from "cap rejected". - `__builtin_set_attributes`: one process-wide `console.warn` when the active world adapter doesn't implement `experimentalSetAttributes`, matching the changelog and `Storage.runs.experimentalSetAttributes` JSDoc. Verified #17 (bare-name step lookup) by precedent: the SWC plugin already special-cases names starting with `__builtin` at `naming.rs`-adjacent path in `lib.rs:1906` to use the bare function name as the step ID, which is exactly how `__builtin_response_json` etc. ship today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(AI) Review feedback addressed —
|
| # | Comment | Resolution |
|---|---|---|
| 11 | world-local mutex coordination — lifecycle writes can clobber attribute writes |
Renamed withRunAttributeLock → withRunFileLock, exported it, and acquire it from the run_started / run_completed / run_failed / run_cancelled branches in events-storage.ts. Each lifecycle write re-reads the run JSON inside the lock before writing, so an experimental_setAttributes call that lands between the lifecycle handler's pre-validation read and write is preserved instead of overwritten. |
| 12 | world-postgres per-run cap not enforced atomically |
Cap check is now inside the same UPDATE statement as the merge: WHERE ... AND (SELECT COUNT(*) FROM jsonb_object_keys(merged_expr)) <= ATTRIBUTE_MAX_PER_RUN. Two concurrent writers adding disjoint keys at the cap boundary can no longer both succeed. A follow-up re-read on rejection disambiguates "run not found" from "cap rejected after concurrent write". |
| 13 | world/attributes.ts cap counts every upsert as +1, including for already-present keys |
validateAttributeChanges now takes existingKeys?: Iterable<string> instead of existingCount?: number. With keys, the check uses real net adds/deletes (an update to an already-present key is zero net). Without keys it falls back to the conservative "every upsert is +1" shape. Added a regression test that exercises the at-the-cap update case. |
| 14 | Silent no-op when world doesn't implement experimentalSetAttributes contradicts docs |
__builtin_set_attributes now emits one process-wide console.warn on first dispatch against an unsupporting world, matching the changelog and the Storage.runs.experimentalSetAttributes JSDoc. The warning is deduped via a globalThis symbol. |
| 16 | Rename to experimental_setAttributes |
Done at every call site: public exports from @workflow/core and workflow, the workflow VM bundle's workflow/index.ts, unit tests, e2e test, workbench example, and changelog. Internal step name stays __builtin_set_attributes (used only by the dispatcher). |
Verified, no change
| # | Comment | Verification |
|---|---|---|
| 17 | Does bare-name '__builtin_set_attributes' step lookup resolve through SWC + step handler? |
Yes — the SWC plugin already special-cases names starting with __builtin at lib.rs:1906 to use the bare function name as the step ID, which is exactly how __builtin_response_json / __builtin_response_text / __builtin_response_array_buffer ship today. Same registry, same dispatch, identical precedent — no integration check needed beyond the existing e2e workflow test. |
Deferred
| # | Comment | Status |
|---|---|---|
| 10 | WORKFLOW_SERVER_URL_OVERRIDE reverted to '' |
Intentional — will revert before merging once the server PR is live. Lint guard correctly catches it. |
| 15 | Workflow-body-only vs. add step-body back | Keeping scope small for the MVP. A first attempt at step-body support clobbered the PR; doing in a follow-up so this lands clean. |
Server-side response: vercel/workflow-server#442 (comment).
…nderLifecycleLock The previous shape took `(baselineRun, overrides)` and spread them inside the helper, which collapses the run's discriminated union (`status: 'pending' | 'running' | 'completed' | ...`) into an unassignable intersection. tsc rejected the call sites in CI. Switch the helper to take an already-constructed `WorkflowRun` and a generic `<T extends WorkflowRun>` so the caller-side narrowing survives. Each lifecycle branch builds the full run object inline (as it did before the lock refactor) and the helper only swaps in the freshest `attributes` snapshot from the on-disk read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| */ | ||
| const WORKFLOW_SERVER_URL_OVERRIDE = ''; | ||
| const WORKFLOW_SERVER_URL_OVERRIDE = | ||
| 'https://workflow-server-git-peter-attributes-mvp.vercel.sh'; |
There was a problem hiding this comment.
(Re-flagging after the latest push — still here.) WORKFLOW_SERVER_URL_OVERRIDE is still set to the preview-branch URL. Vercel bot flagged it; I flagged it; the response commit (f026a5e0b) addressed every other piece of feedback but not this one. Just want to make sure it doesn't get missed before merge — const WORKFLOW_SERVER_URL_OVERRIDE = ''; is the production value.
|
|
||
| If you need behavior the MVP does not provide (read, list, filter, initial attributes at `start`, writer attribution), wait for 5.0.0 rather than building around the MVP surface. | ||
|
|
||
| ## Test plan |
There was a problem hiding this comment.
Documentation and test coverage gaps — particularly around fire-and-forget, which is arguably the canonical usage pattern for observability attributes.
Fire-and-forget pattern is undocumented and untested
Every example in the changelog and JSDoc shows `await experimental_setAttributes({...})`. But for the primary use case (informational/tracking metadata), users almost certainly want to not block the workflow on it:
```ts
export async function processOrder(orderId: string) {
use workflow;
void experimental_setAttributes({ phase: init, orderId }); // fire-and-forget
const validated = await validateOrder(orderId);
void experimental_setAttributes({ phase: validated });
// ...
}
```
The repo already treats this as a first-class pattern — see `packages/core/src/abort-consistency.test.ts:608` (`pending queue items on workflow completion are fire-and-forget`) and `hook-sleep-interaction.test.ts:570`. The drain-on-completion mechanism in `workflow.ts` commits pending step requests even when the workflow body returns without awaiting them, so the attribute write should land — but nothing in this PR verifies that for `experimental_setAttributes` specifically, and the changelog never mentions the pattern as supported.
Suggested test cases (e2e in the workbench):
- `void experimental_setAttributes({...})` followed immediately by `return` → attributes persist on the run row after `run_completed`.
- `void experimental_setAttributes({...})` followed by a throw → attributes persist (the drain runs on the failure path too).
- Mixed: one `await`-ed call followed by a `void` call — both land.
Suggested doc addition (under "Trade-offs and known limitations" or its own section): a paragraph explicitly endorsing the fire-and-forget pattern for observability metadata, with the caveat that the write may land after the run transitions to a terminal status (so list/get callers reading immediately after completion may see attributes flicker in). That tradeoff is real for the world-vercel HTTP path — the step queue runs out-of-band from `run_completed` event processing.
`Promise.all` is warned about but not tested
The JSDoc on `experimental_setAttributes` (`packages/core/src/workflow/set-attributes.ts:25-28`) warns that `Promise.all([experimental_setAttributes(...), experimental_setAttributes(...)])` is "not guaranteed to be ordered consistently." No test verifies the behavior the warning implies — that the workflow VM serializes the calls (it does, since step dispatch is sequential under the VM scheduler) and only the world-side ordering is racy. Worth a workflow.test.ts case that runs the two-call `Promise.all` and asserts the final state matches one of the two legal interleavings, plus an e2e on world-local where the per-run mutex makes the interleaving deterministic.
Stale test plan in the changelog
Three of the listed test cases in §"Test plan" (lines 197-208) dont match the shipped implementation:
- Line 200: "Idempotency: repeated identical calls converge to the same final state" — no such test exists in `runs-storage.test.ts` or anywhere else. It is a worthwhile property to test; should either be added or removed from the plan.
- Line 202: "world-vercel contract test" — no contract test was added; `runs.ts` in `world-vercel` is exercised only transitively via the e2e.
- Lines 205-207: the e2e plan calls for a workflow that "Awaits a step that calls `experimental_setAttributes({ phase: processing, orderId: ord_123 })`" — but step-body is explicitly unsupported and throws `FatalError`. The actual e2e (`setAttributesWorkflow` in `workbench/example/workflows/99_e2e.ts:3184`) is a much simpler workflow-body-only test. Update the plan to match what was shipped.
Missing test cases worth adding
- Workflow throws after `await experimental_setAttributes` — does the attribute persist? It should (per the drain-on-throw path), but its the kind of guarantee users will rely on for crash-tracking attributes.
- Run cancellation mid-setAttributes — what state does the run end in?
- Replay determinism — `experimental_setAttributes` is dispatched as a step, so on workflow restart the step result is replayed from the event log and the side effect (the row write) is NOT re-executed. Worth an explicit test verifying no double-write on replay.
- Step body calls `experimental_setAttributes` — only a unit test covers the host-stub `FatalError`. An e2e verifying the error reaches user code from a real step would close the loop.
- World adapter missing `experimentalSetAttributes` no-ops with a warning — the `__builtin_set_attributes` implementation has the warn-once logic now, but no test verifies (a) the warning fires once, (b) subsequent calls in the same process dont re-warn, (c) the user-facing await still resolves cleanly.
- `runs.list` returns `attributes` — the schema includes it; verify list responses include the field. Otherwise UIs that filter on it via list (not just `get`) silently see nothing.
- Multi-world consistency on the empty case — `world-postgres` defaults the column to `{}` (NOT NULL); `world-local` reads existing JSON files where the field is absent and returns `undefined`. Code that does `run.attributes.foo` works on Postgres and crashes on local. Either align (default `attributes: {}` in the local run reconstruction) or document that callers must use `run.attributes ?? {}`.
Doc nits
- §"Concurrent writes" (line 164) says workflow body writes are serialized "one step at a time within a single workflow body." Thats only true for `await`-ed calls. `Promise.all([...])` produces interleaved step dispatch; fire-and-forget produces a step that runs asynchronously after the workflow body returns. Worth tightening the wording so users dont assume awaited-call semantics generalize.
- Line 282 (implementation notes, end): "End-to-end coverage in the workbench app is intentionally deferred" — but the e2e was added (`packages/core/e2e/e2e.test.ts:3438`). Stale; remove or update to describe what the e2e covers.
- The "warns once" claim now matches the code (good). Consider also documenting the exact warning text and when it fires, so users who do see it in logs have a hit on Google/grep for the SDK source.
2 participants
Summary
Implements the Workflow Attributes MVP — a minimal, write-only attributes API designed to land before the full event-sourced attributes feature in #1933 (which requires a
SPEC_VERSION_CURRENTbump and coordinated rollout across worlds, builders, and runtime).User surface:
Attributes are stored plaintext on the
WorkflowRunentity and visible viaworld.runs.get()/world.runs.list()(and any observability surface built on top). The wire format mirrors the futureattr_setevent'seventData.changes, so the SDK signature and wire body shape are stable across MVP → 5.0.0.setAttributesis callable from a workflow body only. The call is dispatched through an internal__builtin_set_attributesstep bridge so the mutation gets astep_created → step_completedevent pair without inventing a new event type. The host-side export (resolved from step bodies or plain host code) throwsFatalErrordirecting the caller back to a workflow body — step-body support can be added later without breaking the workflow-body contract.See
docs/content/docs/v5/changelog/attributes-mvp.mdxfor the full design, trade-offs, and implementation notes — including decisions made during build-out (endpoint namespacing, concurrency semantics, usage-fact schema choice, optional-world fallback behavior, etc.).Paired with vercel/workflow-server#442 which adds the server-side
POST /api/v2/runs/:runId/attributesendpoint, ElectroDB column, and the newWORKFLOW_ATTRIBUTEusage-fact carrying the post-merge map.What's in this PR
@workflow/worldapplyAttributeChangeshelper. OptionalexperimentalSetAttributesonStorage.runs. Optionalattributesfield onWorkflowRunBaseSchema.@workflow/coresetAttributes(record). VM-side helper validates inline and dispatches via the standardWORKFLOW_USE_STEPmechanism — no new global symbols, no bridge plumbing. Host-side export is aFatalError-throwing stub for non-workflow-body callers.workflowpackage__builtin_set_attributesstep body ininternal/builtins.ts. Reads world + run id directly fromglobalThissymbols populated by the runtime; zero imports from@workflow/coreso it stays a true leaf in the deferred-entries graph.@workflow/world-localattributesthrough the run lifecycle event reconstructions.@workflow/world-postgresattributes jsonbcolumn (migration0013_add_attributes.sql). SQL-side atomic merge usingjsonb_set/-operators in a singleUPDATE.@workflow/world-vercel{ changes: [...] }to/v2/runs/:runId/attributes.docs/content/docs/v5/changelog/attributes-mvp.mdxwith full design + implementation notes.Architecture (workflow-body dispatch)
setAttributes(attrs)from'use workflow'body.workflowpackage-exports condition topackages/core/src/workflow/set-attributes.ts, which validates the input inline and produces canonicalAttributeChange[].globalThis[WORKFLOW_USE_STEP]('__builtin_set_attributes')(changes)— same mechanism every other step call uses.__builtin_set_attributes(inpackages/workflow/src/internal/builtins.ts). The step body reads the world fromglobalThis[Symbol.for('@workflow/world//cache')]and the active run id fromglobalThis[Symbol.for('WORKFLOW_STEP_CONTEXT_STORAGE')], then callsworld.runs.experimentalSetAttributes(runId, changes). No imports from@workflow/core— this is what keeps the Next.js deferred-entries discoverer from walking into world adapters and triggering webpack's regex-extractor stack overflow.What's NOT in this PR (not in MVP)
setAttributesfrom a step body or plain host code (throwsFatalError; can be added later)getAttribute/getAttributes)start(workflow, input, { attributes })(initial attributes at run creation)runs.list({ attributes }),listAttributeKeys,listAttributeValues)attr_setevent type$-prefixed) namespace (just blocked at validation today)