cloudflare / cloudflare/workers-sdk

[workflows] Local `deleteBatch()` always reports success, so failures are invisible

Open
#15,138 3 comments 0 reactions 0 assignees View on GitHub
package:workflows-shared product:workflows
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.