Skip to content

fix(core): treat empty 2xx response bodies as no-content#344

Draft
Butch78 wants to merge 2 commits into
alchemy-run:mainfrom
Butch78:fix/empty-2xx-body
Draft

fix(core): treat empty 2xx response bodies as no-content#344
Butch78 wants to merge 2 commits into
alchemy-run:mainfrom
Butch78:fix/empty-2xx-body

Conversation

@Butch78

@Butch78 Butch78 commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Some Cloudflare endpoints return a success status with an empty body instead of 204 No Content. The clearest case is the Workers custom-domain delete (DELETE /accounts/{account_id}/workers/domains/{domain_id}), which replies 200 with no body — but the OpenAPI-generated DeleteDomainResponse schema still declares a { errors, messages, success: true } envelope (per the documented response). The client only short-circuits the response decode for 204 and for Void-typed operations, so an empty 200 falls through to the JSON decode and fails to satisfy the declared schema.

On its own that is an instant, deterministic error — but a caller that retries on transient errors (for example alchemy's default retry policy) classifies the decode failure as retryable and retries it with exponential backoff, turning it into a multi-hour silent hang on every deploy or destroy that attaches or detaches a custom domain. A direct request to the same endpoint returns a clean 200 with an empty body, confirming the API is healthy and the gap is purely client-side.

Fix

Treat an empty body on a successful DELETE response as no-content, mirroring the existing 204 handling, before attempting to decode it.

Scoped to DELETE (see review discussion) so that an unexpectedly-empty GET/POST/PUT still fails the decode and retries, rather than being silently swallowed — that is the transient-error case we do not want to mask. DELETE callers already receive undefined on the 204/Void paths, so the behavior is consistent, and because the gate only fires on an empty body, DELETEs that legitimately return an envelope (e.g. DNS record delete -> { result: { id } }) are unaffected.

if (
  method === "DELETE" &&
  (rawBody === "" || rawBody === undefined || rawBody === null)
) {
  return outputSchema.ast._tag === "Unknown" ? "" : undefined;
}

References

@Butch78
Butch78 marked this pull request as draft June 14, 2026 11:14
Some endpoints return a success status with an empty body instead of
`204 No Content`. Cloudflare's Workers custom-domain delete
(`DELETE /accounts/{id}/workers/domains/{id}`) is one: it replies `200`
with no body, but the OpenAPI-generated output schema still declares a
`{ errors, messages, success }` envelope.

The client only short-circuits the response decode for `204` and for
`Void`-typed operations, so an empty `200` falls through to the JSON
decode and fails to satisfy the declared schema. Worse, a caller that
retries on transient errors (e.g. alchemy's default retry policy)
classifies that decode failure as retryable and retries it with
exponential backoff — turning an instant, deterministic failure into a
multi-hour silent hang on every deploy/destroy that attaches or detaches
a custom domain.

Treat an empty body on a successful response as "no content", mirroring
the existing `204` handling, before attempting to decode it.
@Butch78
Butch78 force-pushed the fix/empty-2xx-body branch from 5972212 to f5de419 Compare June 14, 2026 11:24
i11v added a commit to i11v/tablo that referenced this pull request Jun 19, 2026
…ins (#18)

## What

Moves tablo's production custom domain to the apex **`tablo.run`** and
gives every PR preview its own **`preview-<N>.tablo.run`** hostname
(previews were `workers.dev`-only before). Also lands the
`@distilled.cloud/core` patch that makes custom-domain deletes (preview
teardown + the production cutover itself) survive a beta CF-client bug.

## Why two commits, in this order

1. **`fix(deploy): patch @distilled.cloud/core empty-body DELETE
crash`** — Cloudflare answers `DELETE /workers/domains/{id}` with an
empty `200` body; the beta CF client falls through to envelope-schema
decode, fails, and aborts the whole `deploy`/`destroy` with the
misleading `CloudflareHttpError: null`, orphaning the worker + its
state. This fires on every custom-domain teardown (the PR-close cleanup
job) **and** on any production domain switch (`reconcileDomains` deletes
the old domain). Patched via `bun patch` to treat an empty/null 2xx body
as no-content, mirroring the client's own `204` branch. `bun install
--frozen-lockfile` re-applies it on every CI runner. Drop once upstream
[alchemy-run/distilled#344](alchemy-run/distilled#344)
ships in a released `@distilled.cloud/core`.

2. **`feat(deploy): serve production on tablo.run, previews on
preview-<N>.tablo.run`** — new `workerDomain()` helper co-located with
`workerName()`: `production → tablo.run`, `pr-<N> →
preview-<N>.tablo.run`, everything else → none. The worker *name* keeps
the `pr-` scheme (`tablo-pr-<N>`); only the public hostname reads
`preview-`. Both `workers.dev` URLs stay live via `url: true`, so the
health smoke test keeps hitting the deterministic `workers.dev` URL
while the PR comment now advertises the custom domain (with
`workers.dev` as a fallback line).

The patch lands first so the production cutover — which detaches
`tablo.i11v.com` — and every preview teardown are both safe.

## Deploy / cutover

Merging triggers the existing CI prod deploy (`--stage production`),
which attaches `tablo.run` and **detaches `tablo.i11v.com`** (replace,
not keep-both). The `tablo.run` zone already exists in the account, so
Cloudflare auto-provisions the proxied DNS record + TLS cert — no
per-deploy dashboard or DNS steps.

## One manual prerequisite (dashboard)

`tablo.run` is apex and Workers Custom Domains *create* the apex DNS
record. Before merging, confirm the `tablo.run` zone has **no
conflicting proxied A/AAAA/CNAME at the apex** (a parking/placeholder
record), or the first prod deploy's domain attach will fail.
`preview-<N>.tablo.run` subdomains are covered by Universal SSL's
`*.tablo.run`.

## Verification

`typecheck` clean, **79/79 unit tests pass** (incl. 4 new `workerDomain`
cases), and the patch was confirmed to re-apply on a clean `bun install
--frozen-lockfile` with alchemy resolving to the patched copy. Live
domain provisioning + teardown against the Cloudflare API only run in CI
— this PR's own preview deploy exercises a `preview-<N>.tablo.run`
attach, and closing it exercises the patched teardown.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01Gp4ZeUYKtUbPkhjM8Gb4TN

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sam-goodwin

Copy link
Copy Markdown
Collaborator

Null response can be a transient error is some cases. Not sure if this is safe? Do we need to instead patch specific APIs known to do this?

Address review feedback: an empty 2xx body is only treated as no-content
for DELETE requests now, instead of every method. A GET/POST/PUT that
comes back unexpectedly empty still fails the schema decode and retries,
so a transient empty response is no longer silently swallowed.

The gate only fires on an empty body, so DELETEs that legitimately
return an envelope (e.g. DNS record delete -> { result: { id } }) are
unaffected; only the documented-200-with-empty-body case (Cloudflare
Workers custom-domain DELETE) is short-circuited.
@Butch78

Butch78 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Good call, that was too broad. Pushed 56b6cb9 to scope the short-circuit to DELETE only.

Reasoning:

  • The transient-empty risk you flagged is real for reads/creates (a GET/POST/PUT that comes back unexpectedly empty should fail the decode and retry, not be silently swallowed). Those paths are now untouched, so they keep failing loudly.
  • The only case we actually need to absorb is a DELETE whose declared schema is a 200 envelope but whose live response is an empty 200. DELETE callers already receive undefined on the existing 204/Void paths, so returning undefined here is consistent.
  • The gate only fires on an empty body, so DELETEs that do return a real envelope (e.g. DNS record delete -> { result: { id } }) decode normally and are unaffected. It is specifically the empty-body case that gets treated as no-content.

On "patch specific APIs instead": the trouble is the discrepancy is between the documented contract and the live wire behavior, not something visible in the spec. The Workers custom-domain delete is documented as 200 + { errors, messages, success }:

...so the generator faithfully emits that envelope, while the live endpoint returns an empty 200 (a direct request to the same endpoint returns a clean 200 with no body). A per-operation allowlist would mean hand-maintaining a list of these as we discover them, with generator support to thread it through. Gating on DELETE + empty-body is a smaller, self-correcting surface that covers the class without masking the dangerous read/create case. Happy to switch to an allowlist if you would rather be fully explicit.

i11v added a commit to i11v/tablo that referenced this pull request Jul 19, 2026
## Summary

Dependency bump across the workspace, verified locally (typecheck, unit
tests, integration suite on local workerd, build, verify:pwa).

| package | before | after |
|---|---|---|
| effect (root, contract, worker, web) | 4.0.0-beta.78 | 4.0.0-beta.99 |
| alchemy (root, worker) | 2.0.0-beta.52 | 2.0.0-beta.63 |
| @effect/platform-node | 4.0.0-beta.78 | 4.0.0-beta.99 |
| @effect/platform-bun (exact devDep) | 4.0.0-beta.78 | 4.0.0-beta.99 |
| @effect/vitest | 4.0.0-beta.78 | 4.0.0-beta.99 |

## API renames (alchemy beta.52 → beta.63)

- `Cloudflare.DurableObjectNamespace<X>()` →
`Cloudflare.DurableObject<X>()` (both DOs)
- `Cloudflare.DurableWebSocket` → `Cloudflare.WebSocket` (session
handlers)

## Patch re-created against @distilled.cloud/core 0.29.1

alchemy@2.0.0-beta.63 pins @distilled.cloud/*@0.29.1, so the
empty-2xx-body guard (alchemy-run/distilled#344, still unmerged as of
2026-07-19) is re-created against 0.29.1. Verified the bug is still
present: 0.29.1 now turns the empty-body decode failure into a
*retryable* TransportError, but the retry re-issues the DELETE and the
operation can still fail — the guard (mirroring the 204 branch) remains
the durable fix. Patched both `src/client.ts` (bun export condition,
what `bunx alchemy` runs) and `lib/client.js` (default condition).

## Drive-by test fix

`packages/worker/test-integration/stack.test.ts`: the plain-GET
`/api/ws` test expected 400, but the 426 upgrade-check branch (added in
#6) runs before the session-id check, so the test could never pass —
broken on main unnoticed because CI doesn't run `test:integration`. Now
expects 426; suite is 3/3 green locally.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01UnK7wXkmdGSShX5AA1TevY

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants