[workflows-shared] Emulate deterministic-ID uniqueness in the local Workflows binding - #14847
[workflows-shared] Emulate deterministic-ID uniqueness in the local Workflows binding#14847TheSaiEaranti wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 7f0e856 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/config
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
commit: |
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Thanks for the PR @TheSaiEaranti 🙂
The changes look good to me, however, could you have a look at the CI failures before we proceed? 🙏
(also once this is green I'll need to ping someone from the workflows team to also validate these changes 🙂)
…orkflows binding The local binding created duplicate executions for deterministic instance ids: create() never checked for an existing instance and createBatch() mapped every input through create(), so code relying on the documented idempotency contract appeared to work locally while double-executing workflow bodies. Match the documented production behavior: create() with an id that already exists throws and retains the existing instance, and createBatch() skips ids that already exist, or repeat within the batch, excluding them from the result and creating instances in batch order. Existence is decided by the engine Durable Object via a new hasInstance() method that reads INSTANCE_METADATA, which is written exactly once by the first init(), so the check is consistent across isolates rather than relying on binding-local state. Auto-generated ids skip the check. Fixes cloudflare#14836
|
Thanks Dario! Rebased on today's main, which picked up the abort-listener cleanup from #14840 cleanly (no conflicts, different code regions). The workflows-shared suite passes locally with the four tests main added since this branch was cut, 157 total, and typecheck is clean. On the CI failures that were showing: they were the vite-8 playground and E2E sets that rotate on re-runs (same families as on my other open PR, where four attempts failed three different unrelated suites). The fresh head from this rebase re-triggers everything, so hopefully this run comes back green on its own. Ready for the workflows team whenever it does. |
30229c0 to
e1aa95c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tic-id contract create() with a caller-provided id now throws once the instance exists, so worker scripts that re-created the same fixed id on every request switch to the pattern production code needs anyway: try create, fall back to get for the existing instance. Covers the miniflare workflows persistence spec (re-created "an-id" in a polling loop and again after a restart), the dev-registry cross-worker workflow script, and both vite-plugin workflow playgrounds (whose only repeated-create path is a vitest retry against the still-running server).
|
@dario-piotrowicz I need to correct part of my earlier CI analysis. One of the Aug 6 failures was real and caused by this PR, not flake. The miniflare test "persists Workflow data on file-system between runs" drives a worker that calls create with the same fixed id on every request, in a polling loop and again after restarting Miniflare with the same persistence path. Under the new contract the second create throws, so the test failed identically on Linux and macOS. I missed it because it sat in a rerun of the stale July 25 head where nearly everything else was failing for an unrelated reason. Fixed in cbccc99. The test workers now try create and fall back to get when the id already exists, which is what user code has to do against production Workflows anyway. Reading the persisted status through get after the restart is also a more direct check of persistence than the old silent re-create. I swept the rest of the repo for repeated-create sites and gave the same treatment to the dev-registry cross-worker workflow script and the two vite plugin workflow playgrounds, which could only hit this through a vitest retry against a still-running server. The workflow fixtures and vitest-pool-workers examples create each fixed id once per run and are unaffected. On the rest of the Aug 6 failures: that rerun executed the July 25 commit, whose pinned workerd only supports compatibility dates up to 2026-07-29. Every test that starts a dev server without a pinned compatibility date defaults to the current date, so all fixtures jobs, all wrangler e2e shards, and all vite playground jobs failed with "The Workers runtime failed to start". Those clear on the rebased head. Locally on cbccc99: the miniflare workflows spec passes 5/5, the dev-registry workflow test passes, both playground workflow specs pass, and the workflows-shared suite, typecheck, and formatting are clean. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…plicate-id checks
create(): restore the destructuring defaults. The ?? refactor was not
equivalent: an explicit null id became a generated id instead of being
rejected by validation, and an explicit null params was rewritten to {}.
createBatch(): validate every id before any existence probe or create.
Probing an id constructs the engine Durable Object, whose constructor
persists storage, so an invalid id previously left a phantom instance on
disk and rejected the batch only after earlier entries were already
created. A batch with a malformed id now creates nothing.
createBatch(): skip an id that a concurrent create claims between the
existence check and create(), per the skip contract, instead of failing
the batch part way through.
Three regression tests, each verified to fail on the parent commit.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ded form Every other instance error here goes through createWorkflowError(message, code); the duplicate-id error was a free-form sentence, so code branching on an error code would behave differently locally than against the real service. Route it through a duplicateInstanceError(id) helper with the code instance.already_exists, following the instance.not_found vocabulary, and replace createBatch's substring matcher with a shared isDuplicateInstanceError predicate so the thrower and matcher stay in sync. The race regression test throws through the same helper.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e-id error The catch-all fallbacks treated every create() failure as "the id already exists", so a real failure (malformed id, transient RPC error) was masked by the instance.not_found error from the follow-up get(). Match the instance.already_exists code and rethrow everything else, in both playground workers and both miniflare test worker scripts.
| const results: { id: string }[] = []; | ||
| const seenIds = new Set<string>(); | ||
| for (const options of batch) { | ||
| const id = options.id; | ||
| if (id !== undefined) { | ||
| if (seenIds.has(id) || (await this.#instanceExists(id))) { | ||
| continue; | ||
| } | ||
| seenIds.add(id); | ||
| } | ||
| try { | ||
| results.push(await this.create(options)); | ||
| } catch (e) { | ||
| // A concurrent create can claim the id between the existence | ||
| // check above and create(); the batch contract skips such ids | ||
| // rather than failing the batch. | ||
| if (id !== undefined && isDuplicateInstanceError(e)) { | ||
| continue; | ||
| } | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Creating a batch of workflows now checks every id twice and runs one at a time
Each entry in a batch is checked for an existing instance twice — once in the batch loop and again inside the per-instance creation call (this.#instanceExists(id) at packages/workflows-shared/src/binding.ts:268 plus the same check inside create() at packages/workflows-shared/src/binding.ts:165) — and the entries are now started strictly one after another instead of together, so a large batch takes noticeably longer to start locally.
Impact: Local development batches (up to 100 workflow instances) take many more sequential round-trips to start than before, slowing down local runs.
Sequential loop with duplicated existence probe replaces the previous parallel Promise.all
Previously createBatch() fanned out with Promise.all(batch.map(...)). The new implementation awaits #instanceExists() for every entry with an id, and then awaits this.create(options), which itself performs the identical #instanceExists() RPC against the same engine Durable Object (packages/workflows-shared/src/binding.ts:165-167). That is two RPCs per entry, executed strictly sequentially, i.e. up to 200 serialized Durable Object round-trips for a 100-item batch.
The within-batch de-duplication (seenIds) and the already-exists skip require checking before calling create(), but the second probe inside create() is redundant for the batch path — the batch loop already knows the answer. One option is an internal creation helper that takes an "already checked" flag (or accepts a pre-computed existence result), and keeping the per-entry work concurrent where the de-duplication semantics allow it (e.g. resolving all existence probes in parallel first, then creating the surviving entries in batch order).
Prompt for agents
In packages/workflows-shared/src/binding.ts, createBatch() now iterates the batch strictly sequentially and calls #instanceExists() for every entry with an id, then calls this.create(options), which performs the very same #instanceExists() probe again (see the duplicate-id guard added in create()). For a 100-entry batch this is up to 200 serialized Durable Object RPC round-trips, whereas the previous implementation created all entries concurrently with Promise.all. Consider factoring the creation logic into an internal helper that can skip the redundant existence probe when the caller has already performed it (e.g. a private #createUnchecked(options) used by both create() and createBatch()), and consider resolving the existence probes concurrently before performing the ordered creations, so the batch path does not regress in latency while still preserving the ordering, within-batch de-duplication, and skip-existing semantics.
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Fixes #14836.
The local Workflows binding did not emulate the documented deterministic-ID uniqueness contract, in the dangerous direction: code relying on "duplicate create with the same ID is safely deduplicated" appeared to work locally while double-executing workflow bodies.
This PR matches the documented production behavior:
create({ id })with an ID that already exists throws (Workflow instance with id "<id>" already exists) and the existing instance is retained.createBatch()skips IDs that already exist — or that repeat within the batch — excluding them from the result, and creates instances in batch order. All IDs in a batch are validated before anything is probed or created, so a batch containing a malformed ID is rejected without partial application, and an ID claimed by a concurrent create between the existence check and creation is skipped rather than failing the batch.crypto.randomUUID()) IDs skip the existence check; their behavior is unchanged.How existence is decided: a new
Engine.hasInstance()RPC readsINSTANCE_METADATAfrom Durable Object storage, which is written exactly once by the firstinit()for the ID. Consulting the engine DO keeps the answer consistent across isolates rather than relying on binding-local state, and the DO's input gates order a sequential duplicatecreate()after the firstcreate()'sinit()dispatch, so the sequential case in the issue's repro is deterministic.init()itself is untouched — it is intentionally re-entrant (the engine calls it on restart/wake paths).One boundary note: two
create()calls for the same ID racing in the same tick (Promise.all) remain best-effort, since fully closing that window would require moving creation into a single engine RPC and changinginit()'s contract. The issue's repro (sequential awaited creates) is fully handled.Tests: seven new cases in
binding.test.ts— duplicatecreate()throws;createBatch()excludes an existing ID (returns[], and[fresh]for a mixed batch); within-batch duplicates collapse to one; auto-generated IDs unaffected. Fullworkflows-sharedsuite: 160 passed.tscclean, oxfmt applied.