Skip to content

fix(pg-cloudflare): emit write errors that have no callback - #3784

Open
askalf wants to merge 1 commit into
brianc:masterfrom
askalf:fix/cf-socket-surface-write-errors
Open

askalf wants to merge 1 commit into
brianc:masterfrom
askalf:fix/cf-socket-surface-write-errors

Conversation

@askalf

@askalf askalf commented Sep 14, 2026

Copy link
Copy Markdown

Addresses the diagnosability half of #3757 (it does not claim to fix the hang itself; see below).

Summary

  • CloudflareSocket.write() declared callback: (error?: unknown) => void = () => {}. Connection._send() (packages/pg/lib/connection.js:175) calls this.stream.write(buffer) with no callback, so that no-op default swallowed every rejection from WritableStreamDefaultWriter.write().
  • A failed write therefore produced no error anywhere: the query sat unsettled until the socket closed on its own, and the application saw only the generic post-close Connection terminated unexpectedly.
  • Fix: make callback optional and, when none was supplied, this.emit('error', err) — which is what a net.Socket does and what the rest of pg is already wired for (ConnectionClient._handleErrorEvent_errorAllQueries). Writes that do pass a callback are unchanged and still get the error there.
  • Two regression tests next to the existing mock-writer tests in packages/pg-esm-test/pg-cloudflare.test.js: the emit path, and the callback path (so this can't regress the write(data, callback) support from fix: support callback as second argument to CloudflareSocket.write #3747).
$ # --- on base (9683053), regression test present, fix NOT applied ---
$ node --test --conditions=workerd pg-cloudflare.test.js
▶ pg-cloudflare
  ✔ should export CloudflareSocket constructor (8.422557ms)
  ✔ should safely end after the underlying socket has closed (2.069587ms)
  ✔ should call the write(data, callback) callback exactly once (1.215912ms)
  ✖ should emit error when a write without a callback fails (102.766168ms)
  ✔ should report a failed write to the callback rather than emitting error (4.22798ms)
  ✔ should emit close when ending a socket whose closed promise never settles (2.411796ms)
ℹ tests 6
ℹ pass 5
ℹ fail 1
✖ should emit error when a write without a callback fails (102.766168ms)
  Error: error event was not emitted

$ # --- with the fix applied ---
$ node --test --conditions=workerd pg-cloudflare.test.js
▶ pg-cloudflare
  ✔ should export CloudflareSocket constructor (5.316283ms)
  ✔ should safely end after the underlying socket has closed (1.46093ms)
  ✔ should call the write(data, callback) callback exactly once (1.116075ms)
  ✔ should emit error when a write without a callback fails (1.437625ms)
  ✔ should report a failed write to the callback rather than emitting error (0.618606ms)
  ✔ should emit close when ending a socket whose closed promise never settles (0.963671ms)
ℹ tests 6
ℹ pass 6
ℹ fail 0

tsc -p packages/pg-cloudflare/tsconfig.json, prettier --check and eslint on the two files: all clean.

Decisions

  • Optional parameter instead of a no-op default. With a default callback the code can't tell "caller wants the error" from "caller passed nothing"; making it optional is what makes the distinction expressible. emit('error') fires only in the branch that was previously a silent drop; every other path is byte-for-byte what it was. end() passes its own callback, so its handling is untouched (the existing close test still passes).
  • Not chunking large writes. That was the original hypothesis on g-cloudflare: large single statement (~124KB) reliably kills the connection on Cloudflare Workers - "Connection terminated unexpectedly" #3757, but it can't be proven without a deployed Workers runtime, and @charmander said on the issue they doubt backpressure is the mechanism. Shipping a chunk size would be guessing. This change makes the runtime's actual rejection visible instead, which is the information g-cloudflare: large single statement (~124KB) reliably kills the connection on Cloudflare Workers - "Connection terminated unexpectedly" #3757 is missing: whoever reproduces it on a Worker with this patch should now see the real error rather than "Connection terminated unexpectedly".
  • Not touching Connection._send() (pass a callback there): larger blast radius across all transports for a defect in the Workers socket's own contract.
  • Not setting writable = false on failure: Client._handleErrorEvent already marks the client unusable once the error is emitted; kept to one change.

Not run here: the full Node × PostgreSQL matrix (needs a live server; this touches only the Workers socket) and the packages/pg/test/cloudflare vitest-pool-workers suite (needs wrangler/workerd). #3718 is the only open PR touching this file; it changes end(), not write(), and is already superseded by #3735.

AI assistance: the bug was traced and the fix and tests were drafted with AI tooling in my workflow, starting from @charmander's comment on #3757; the tests, tsc, prettier and eslint were executed as pasted above. I'm responsible for the change and will handle review feedback.

`Connection._send()` calls `stream.write(buffer)` with no callback, so
`CloudflareSocket.write()` fell back to its no-op default callback and a
rejected `WritableStreamDefaultWriter.write()` was discarded. The query
then hung until the socket closed and the real failure surfaced only as
"Connection terminated unexpectedly".

Emit the rejection as an `error` event when no callback was supplied,
matching what a `net.Socket` does, so `Connection` reports it. Writes
that do pass a callback keep receiving the error there.
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.

1 participant