cloudflare / cloudflare/workers-sdk
[workflows] Local `deleteBatch()` always reports success, so failures are invisible
- Dominant language
- TypeScript
- Stars
- 4.5k
- Forks
- 1.5k
- Avg merge
- 3d 8h
- Merged PRs (30d)
- 186
Description
Split out of #15123 so that PR can land. Raised by Devin: https://github.com/cloudflare/workers-sdk/pull/15123#discussion_r3756702759
### Problem
In `WorkflowBinding.deleteBatch()` (`packages/workflows-shared/src/binding.ts:243-309`), the returned `errors` array can never be populated, so local dev always reports a batch delete as fully successful — including for instance IDs that do not exist. That diverges from production behaviour and hides real failures.
Three separate reasons, all verified against the current code:
**1. Both branches record success** (`binding.ts:274-279`):
```ts
try {
await stub.getStatus();
resultMap.set(id, { ok: true });
} catch {
resultMap.set(id, { ok: true });
}
```
The `getStatus()` result is discarded either way, so it is a wasted RPC round-trip. It is also being called on a stub that has just been aborted by `stub.unsafeAbort()` on the line above — the stub is stale at that point (compare the retry handling around `binding.ts:400-421`), so this call is expected to reject in the normal case.
**2. The rejection sweep is unreachable** (`binding.ts:283-292`). Every `await` inside the `Promise.allSettled` callback is already wrapped in `try`/`catch` (`binding.ts:268-272` and `274-279`), so the callback cannot reject and `result.status === "rejected"` is never true.
**3. A missing result is also counted as deleted** (`binding.ts:297-306`). The final loop's `else` branch pushes `{ id }` into `deleted` when `resultMap` has no entry for the ID at all, which masks any path that failed to record a result.
### Also in the same function
IDs are de-duplicated for the delete work (`uniqueIds`, `binding.ts:258`) but the result is built by iterating the original `instanceIds` (`binding.ts:297`), so a caller passing the same ID twice gets it back twice in `deleted`.
### What needs deciding
The intended semantics, before the code can be restructured:
- Should a non-existent instance be an error entry (`not_found`) or a silent success? Production behaviour should be the reference here.
- If it should be an error, existence has to be checked *before* `unsafeAbort()`, e.g. `getStatus()` on a fresh stub first, since after the abort the stub cannot be used to tell.
- Should duplicate input IDs appear once or once-per-occurrence in `deleted`?
### Acceptance
- A genuine failure (and, if that is the chosen semantics, an unknown instance ID) lands in `errors` rather than `deleted`.
- No discarded RPC calls left behind.
- Duplicate-ID behaviour is deliberate and covered by a test (see #15139).
Contributor guide
Research direction
Start in packages/workflows-shared/src/binding.ts, at WorkflowBinding.deleteBatch() lines 243-309, and compare its retry handling around lines 400-421. Review the production behavior and the test planned in #15139, then confirm that genuine failures and the chosen unknown-ID semantics reach errors, discarded RPC calls are absent, and duplicate-ID behavior is covered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- backend, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100