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

[petebacondarwin]

Split out of #15123 so that PR can land. Raised by Devin: #15123 (comment)

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):

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 [workflows] Local delete() and deleteBatch() have no test coverage #15139).

[workers-devprod]

• Reproduction: ✅ Successfully reproduced. Checked out PR Bump the workerd-and-workers-types group across 1 directory with 2 updates #15123 where deleteBatch() was introduced and added targeted tests to packages/workflows-shared/tests/binding.test.ts. A non-existent instance ID was returned in deleted with errors empty, and duplicate input IDs produced two entries in deleted instead of one. Both assertions failed exactly as described in the issue.

[workers-devprod]

Note

This is an automated, advisory triage report generated by workers-devprod. It is not an official maintainer response — a maintainer will follow up.

🤖 Automated triage report

Issue Triage: #15138

Summary

In local dev, WorkflowBinding.deleteBatch() always reports every instance ID as successfully deleted (the errors array can never be populated), diverging from production and hiding real failures.

Classification

• Type: Bug
• Component: packages/workflows-shared
• Severity: Medium
• Has Reproduction: N/A (code-level bug report with exact file/line references)
• Quality: Complete

Findings

• Created: 2026-08-11
• Author: petebacondarwin (maintainer)
• Version: not specified (bug identified directly against current source)
• Labels: product:workflows
• Comments: 2+ (content not recoverable from pre-fetched data; none authored by the reporter)

Analysis

• Maintainer-filed bug, split out of PR Bump the workerd-and-workers-types group across 1 directory with 2 updates #15123 and originally raised by the Devin review bot (ref: #15123#discussion_r3756702759). This is a tracking issue for a known, verified defect, not a user-reported problem needing reproduction.
• Component is unambiguous: packages/workflows-shared/src/binding.ts, function deleteBatch() (lines ~243–309). The product:workflows label matches; per the component map, Workflows code lives in workers-sdk (workflows-shared + wrangler).
• The report documents three concrete defects, each with line references: (1) both the try and catch branches record { ok: true }, wasting an RPC on an already-aborted stub (binding.ts:274-279); (2) the rejection sweep in the Promise.allSettled callback is unreachable because every await is already wrapped in try/catch (binding.ts:283-292); (3) a missing resultMap entry is still counted as deleted (binding.ts:297-306).
• Additional defect noted: 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 duplicate input IDs are returned twice in deleted.
• The body ends with a "What needs deciding" section (intended semantics of the result). The exact decision text was not recoverable from the truncated pre-fetched data, so a maintainer should confirm the desired behaviour (e.g. how non-existent IDs and duplicates should be reported) before/while implementing.

Recommendation

Status: KEEP OPEN

Reasoning: This is a well-specified, maintainer-filed bug with exact source references and clear root-cause analysis. It is a legitimate, actionable defect in packages/workflows-shared that correctly diverges local dev behaviour from production, and it is already scoped as follow-up work from PR #15123.

Action: Keep open as a tracking item for the workflows team. Confirm the intended result semantics (handling of non-existent and duplicate instance IDs) from the "What needs deciding" section, then fix deleteBatch() so real failures surface in local dev and the result is built from the de-duplicated ID set.

Suggested Labels: product:workflows

Suggested Comment

Thanks for splitting this out. The analysis looks correct — in deleteBatch() the errors array can never be populated, so local dev always reports full success, including for instance IDs that don't exist. Before we implement, could we confirm the intended result semantics from the "What needs deciding" section — specifically how non-existent instance IDs should be reported and whether duplicate input IDs should be collapsed in the result? Once that's settled we can fix the success/error accounting and build the result from the de-duplicated ID set.

Structured summary (JSON)

{
  "issueNumber": 15138,
  "title": "[workflows] Local deleteBatch() always reports success, so failures are invisible",
  "githubUrl": "https://github.com/cloudflare/workers-sdk/issues/15138",
  "recommendation": "KEEP OPEN",
  "difficulty": "medium",
  "reasoning": "Well-specified, maintainer-filed bug in packages/workflows-shared with exact source references showing deleteBatch() can never report errors in local dev. It is a legitimate, actionable defect split out of PR #15123.",
  "suggestedAction": "Keep open as a tracking item for the workflows team; confirm intended result semantics for non-existent and duplicate instance IDs, then fix the success/error accounting in deleteBatch() and build the result from the de-duplicated ID set.",
  "hasSuggestedComment": true,
  "suggestedLabels": ["product:workflows"]
}

[ryux1]

I would like to take this.

The current workerd API contract already resolves both open questions:

• The public type docs say duplicate IDs are deleted once, while the result keeps one entry per input position: https://github.com/cloudflare/workerd/blob/main/types/defines/workflows.d.ts#L38-L45
• The HTTP and RPC contract tests both expect an unknown ID in errors with code 10400 and message workflows.api.error.instance.not_found, while a duplicated successful ID appears twice in deleted: https://github.com/cloudflare/workerd/blob/main/src/cloudflare/internal/test/workflows/workflows-api-rpc-test.js#L35-L50

I propose matching that contract locally: deduplicate the actual delete work, check each fresh stub before aborting it, record the established not-found payload when that check fails, remove the discarded post-abort status call and unreachable allSettled sweep, then construct the result from the original input so positional duplicate behavior is preserved. I will include regression coverage for existing, missing, duplicate, empty, and over-limit batches.