Revert "[Workflows] Implement Workflows instance methods"#12872
Revert "[Workflows] Implement Workflows instance methods"#12872edmundhung merged 1 commit intomainfrom
Conversation
This reverts commit 8d4ef78.
|
|
Good - both LGTM |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
LGTM from the workflows team |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| await this.storeEventMap(); | ||
| // TODO: persist eventMap - it can be over 2MiB | ||
| this.eventMap.set(event.type, eventTypeQueue); |
There was a problem hiding this comment.
🔴 receiveEvent calls storeEventMap before adding event type to eventMap, losing first events of new types
In receiveEvent(), the order of this.eventMap.set() and this.storeEventMap() was swapped compared to the original code (pre-PR #12814). When a new event type is first received, eventTypeQueue is a brand-new array ([] from the ?? [] fallback) that is NOT yet in this.eventMap. The event is pushed to this detached array, then storeEventMap() iterates this.eventMap.entries() — which doesn't include the new type — so the event is never persisted to storage. The this.eventMap.set() call only happens afterward. If the DO restarts before the event is consumed in-memory, the event is permanently lost, causing waitForEvent to hang until timeout.
Comparison with original code (pre-PR #12814)
Original order:
this.eventMap.set(event.type, eventTypeQueue);
await this.storeEventMap();
New (incorrect) order:
await this.storeEventMap();
this.eventMap.set(event.type, eventTypeQueue);
| await this.storeEventMap(); | |
| // TODO: persist eventMap - it can be over 2MiB | |
| this.eventMap.set(event.type, eventTypeQueue); | |
| this.eventMap.set(event.type, eventTypeQueue); | |
| await this.storeEventMap(); | |
| // TODO: persist eventMap - it can be over 2MiB |
Was this helpful? React with 👍 or 👎 to provide feedback.
| public async status(): Promise<InstanceStatus> { | ||
| using instance = await this.getInstance(); | ||
| const instance = (await this.binding.get(this.id)) as WorkflowInstance & | ||
| Disposable; | ||
| using res = (await instance.status()) as InstanceStatus & Disposable; | ||
| instance[Symbol.dispose](); | ||
| return structuredClone(res); | ||
| } |
There was a problem hiding this comment.
🟡 InstanceImpl.status() and sendEvent() leak RPC handle on exception path
In the miniflare wrapped binding, InstanceImpl.status() and InstanceImpl.sendEvent() replaced the original RAII using instance = await this.getInstance() pattern with manual instance[Symbol.dispose]() calls. If the intermediate operation (instance.status() or instance.sendEvent()) throws an exception, instance[Symbol.dispose]() is never reached, leaking the RPC handle. The original code at packages/miniflare/src/workers/workflows/wrapped-binding.worker.ts (pre-PR #12814) correctly used using declarations that guarantee disposal even on exception paths.
| public async status(): Promise<InstanceStatus> { | |
| using instance = await this.getInstance(); | |
| const instance = (await this.binding.get(this.id)) as WorkflowInstance & | |
| Disposable; | |
| using res = (await instance.status()) as InstanceStatus & Disposable; | |
| instance[Symbol.dispose](); | |
| return structuredClone(res); | |
| } | |
| public async status(): Promise<InstanceStatus> { | |
| using instance = (await this.binding.get(this.id)) as WorkflowInstance & | |
| Disposable; | |
| using res = (await instance.status()) as InstanceStatus & Disposable; | |
| return structuredClone(res); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
edmundhung
added
the
skip-pr-description-validation
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
CI is green so it looks like this was the cause of our previous failures on main...
edmundhung
deleted the
revert-12814-osilva/implement-workflows-instance-methods
branch
This reverts commit 22b51cd.
This reverts commit 22b51cd.
This reverts commit 22b51cd.
This reverts commit 22b51cd.
This reverts commit 22b51cd.
This reverts commit 22b51cd.
4 participants
Reverts #12814
We are now seeing constant CI errors in the
Tests (Windows)CI job (e.g. https://github.com/cloudflare/workers-sdk/actions/runs/23015495128/job/66849585138?pr=12840)I am not 100% sure it's related to this change. We will see if the CI job pass in this PR.