[workflows-shared] Fix Uint8Array step outputs dragging backing ArrayBuffer in local Workflows#14118
Conversation
🦋 Changeset detectedLatest commit: 09c80f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
…step outputs Addresses review feedback on cloudflare#14118. normalizeForStorage is now a recursive walker (cycle-safe via WeakMap) that descends into arrays, plain objects, Maps, and Sets, compacting typed-array views wherever they appear in the value tree. Top-level-only normalisation missed nested views (e.g. `{ image: Uint8Array }`). Compaction now preserves the original view constructor instead of flattening every view to ArrayBuffer. Uint8Array stays Uint8Array, Int16Array stays Int16Array, DataView stays DataView, etc. The persisted shape matches the live shape, so cached replays observe the same constructor the step originally returned. Added regression tests: Int16Array round-trip on the live path, a sliced view nested in a plain object, and sliced views nested two levels deep in an array of objects. The original top-level tests still pass.
|
Both updated in 40faabceb. Lmk if anything else. |
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
@cloudflare/wrangler-bundler
commit: |
|
@Caio-Nogueira - can you take another look? |
…tructor Per Caio-Nogueira's review suggestion on cloudflare#14118. Replaces the 13-branch instanceof dispatch with a typed cast on view.constructor, and uses view.buffer.slice() directly for the tight copy. Same behaviour, much shorter.
|
Codeowners approval required for this PR:
Show detailed file reviewers |
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
@emily-shen @Caio-Nogueira thanks for the approvals. |
…Buffer in local Workflows
…step outputs Addresses review feedback on cloudflare#14118. normalizeForStorage is now a recursive walker (cycle-safe via WeakMap) that descends into arrays, plain objects, Maps, and Sets, compacting typed-array views wherever they appear in the value tree. Top-level-only normalisation missed nested views (e.g. `{ image: Uint8Array }`). Compaction now preserves the original view constructor instead of flattening every view to ArrayBuffer. Uint8Array stays Uint8Array, Int16Array stays Int16Array, DataView stays DataView, etc. The persisted shape matches the live shape, so cached replays observe the same constructor the step originally returned. Added regression tests: Int16Array round-trip on the live path, a sliced view nested in a plain object, and sliced views nested two levels deep in an array of objects. The original top-level tests still pass.
…tructor Per Caio-Nogueira's review suggestion on cloudflare#14118. Replaces the 13-branch instanceof dispatch with a typed cast on view.constructor, and uses view.buffer.slice() directly for the tight copy. Same behaviour, much shorter.
After cloudflare#14134 merged the binaryReplacer fix into engine.ts:writeLog, a 2 MB Uint8Array step output no longer terminates the workflow — the documented 1MiB step-output cap isn't enforced at this layer post-fix. This regression test was asserting that side-effect; remove it. The remaining typed-array tests still validate the deep-walk + view-type preservation behaviour that this PR adds.
10dd32a to
09c80f8
Compare
Fixes #14101. Related: #10966 (Workflows local-dev / production behaviour alignment).
Problem
In local
wrangler dev, aUint8Arrayreturned from a Workflows step fails withstring or blob too big: SQLITE_TOOBIGat sizes well below the documented 1MiB step-output limit. The same bytes wrapped as a tightArrayBuffersucceed up to ~2MB; production accepts theUint8Arrayeither way. Reporter's turn-key repro: https://github.com/danieltroger/cf-workflows-local-sqlite-toobigThe cause is that
v8::ValueSerializer(used by Durable Object SQL storage in workerd) writes the entire backingArrayBufferfor typed-array views, not justbyteLengthbytes. When a view is created on a larger buffer —crypto.getRandomValues(new Uint8Array(N)),arr.slice(...)on a bigger pool, fetch-stream copies, etc — the wire size balloons by a factor of (backing-size / view-size).Fix
packages/workflows-shared/src/context.ts— a smallnormalizeForStoragehelper copies typed-array views into a tightArrayBufferbeforestorage.put:Applied in
persistStepResultimmediately before the storage write. The caller (the user'sstep.do(...)) continues to receive the originalUint8Array; only the on-disk form changes.Files
packages/workflows-shared/src/context.ts—normalizeForStoragehelper + integration inpersistStepResultpackages/workflows-shared/tests/context.test.ts— four new regression tests underContext - typed-array step outputs (issue #14101): tight-buffer success, sliced-buffer success (the reporter's exact repro), oversize-rejection, round-trip live-path-shape.changeset/fix-workflows-uint8array-step-output.md—miniflare+wranglerpatchTest plan
wrangler devbehaviour in line with already-documented production behaviour (1MiB step-output limit)Local validation:
pnpm test:ci -F @cloudflare/workflows-shared— 117/117 passing including the four new regression testspnpm run check:lint— cleanpnpm prettify— cleanpnpm check:type(workflows-shared) — cleanThe repo-wide
pnpm checkreports a pre-existingcheck:package-depsfailure unrelated to this PR (wrangler-imports-vite-without-declaring); confirmed reproducible on a cleanupstream/maincheckout viagit stash && pnpm check.Notes for reviewer
{ image: Uint8Array, meta: {...} }would still suffer the bloat at the nested level. Happy to extend to a deep-walk (arrays + objects +Map/Setvalues) — left top-level only to match the smallest scope that addresses the headline bug, but I'd appreciate confirmation on whether to extend.ArrayBuffer, which means cached-replay reads also surfaceArrayBuffereven if the original step returned aUint8Array. The live execution path is unchanged (the caller'sstep.do()still sees the originalUint8Array). If production rehydrates toUint8Arrayon replay, flag it and I'll add a side-channel__viewKindmarker plus decoder.WorkflowInternalError("Maximum allowed size is 1MiB.")atcontext.ts:768-774does NOT actually fire for typed-array outputs — the rawSQLITE_TOOBIGreachesWORKFLOW_FAILUREinstead. The error surfaces asynchronously from the workerd DO storage output gate, bypassing the awaited try/catch aroundpersistStepResult. The 2MB oversize-rejection test documents this and asserts on the contract (terminal failure) rather than the message wording. Happy to file a separate follow-up issue with a more invasive fix once this lands; out of scope here to keep the change focused.A picture of a cute animal (not mandatory, but encouraged)