Skip to content

test(node): clearImmediate semantics across TCP socket close - #34852

Merged
littledivy merged 1 commit into
mainfrom
orch/divybot-465
Jun 5, 2026
Merged

test(node): clearImmediate semantics across TCP socket close#34852
littledivy merged 1 commit into
mainfrom
orch/divybot-465

Conversation

@divybot

@divybot divybot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a spec regression test that pins down setImmediate / clearImmediate
semantics around a TCP socket's 'close' event. The test directly mirrors the
write-batching pattern that surfaces in the crash reported in #34667 (and the
duplicate trail of #29262 sightings against npm:postgres):

function write(x) {
  chunk = chunk ? Buffer.concat([chunk, x]) : Buffer.from(x)
  if (nextWriteTimer === null)
    nextWriteTimer = setImmediate(nextWrite)
}
function nextWrite() {
  socket.write(chunk, fn)               // ← crashes when socket === null
  nextWriteTimer !== null && clearImmediate(nextWriteTimer)
  chunk = nextWriteTimer = null
}
function closed() {
  clearImmediate(nextWriteTimer)
  socket = null
}

Investigation

I reproduced the exact stack trace from #34667 with a minimal pure-node:net
script (no npm:postgres needed) and confirmed it crashes identically on
both Deno and Node.js 24.15.0
— same stack, same TypeError: Cannot read properties of null (reading 'write') from inside Immediate.callback. So
the symptom is not Deno-specific; it's a userland race in postgres-js's
connection-pool reuse path that surfaces on every libuv-compatible runtime.

The root cause (already triaged upstream as porsager/postgres#1066 and #1154):

  1. The slow query's write() queues setImmediate(nextWrite). That immediate
    runs once, in the check phase of the same tick — long before any
    server-side close arrives. nextWriteTimer is reset to null inside the
    immediate.
  2. The server kills the backend. EOF arrives, the socket destroys, the close
    callback emits 'close'. postgres-js's closed() calls
    clearImmediate(nextWriteTimer) — but the timer is already null, so it's
    a no-op — then sets socket = null.
  3. The slow query's promise rejects. The await slow continuation runs in the
    microtask drain that follows the close handler, then the user calls
    conn.release() → which moves the now-dead connection into the pool's
    open queue → sql.reserve() shifts it back out → the next INSERT
    triggers c.execute(q)write(insertBytes)a fresh
    setImmediate(nextWrite) is queued.
  4. That fresh immediate fires on the next tick. By then socket === null,
    so socket.write(chunk, fn) throws — and because the throw is inside a
    check-phase callback, no surrounding try/catch in user code can catch
    it. The process exits.

Step 3 is the actual bug. The upstream fix is porsager/postgres#1168, which
adds a socket && socket.write(chunk, fn) guard inside nextWrite. It's been
open since 2026-05-25 awaiting maintainer review, with at least one user already
applying it as a pnpm patch to keep their service stable.

What this PR adds

A spec test under tests/specs/node/clear_immediate_socket_close_race/ that
pins down four invariants Deno already satisfies, so we never silently regress
against the actual contract reporters were expecting:

  1. clearImmediate(t) called synchronously cancels t.
  2. clearImmediate(t) called from a microtask before any check phase runs
    cancels t.
  3. When a setImmediate is queued in the same tick that the socket is torn
    down, libuv's check phase runs before the close phase — so the queued
    immediate fires first and the close handler's clearImmediate is a no-op,
    identical to Node.js. (The reporter assumed the opposite order, which is
    the heart of the misdiagnosis.)
  4. With a socket && … guard inside nextWrite (matching upstream Rewrite libdeno/snapshot_creator.cc in Rust #1168),
    the pool-reuse-after-close pattern stops crashing on Deno.

What this PR does NOT fix

The actual crash for users still on postgres@3.4.9 (or anything pre-#1168).
That fix has to land in porsager/postgres and propagate via an npm release —
Deno can't patch user libraries in-tree. Pointing the issue at upstream so users
can apply the pnpm patch workaround in the meantime.

Test plan

  • tests/specs/node/clear_immediate_socket_close_race/ passes locally.
  • Removing the socket && … guard from case 4 reproduces the exact
    Uncaught TypeError: Cannot read properties of null (reading 'write')
    stack from the issue.
  • The same un-guarded script crashes identically on Node.js 24.15.0,
    confirming this is not a Deno-specific bug.
  • deno fmt + deno lint clean on the new file.

Refs #34667
Refs porsager/postgres#1168

Closes denoland/divybot#465

Add a spec test that mirrors the `npm:postgres` (postgres-js) write
batching pattern that's been surfacing in user crashes:

  function write(x) {
    chunk = concat(chunk, x)
    if (nextWriteTimer === null)
      nextWriteTimer = setImmediate(nextWrite)
  }
  function nextWrite() {
    socket.write(chunk)
    clearImmediate(nextWriteTimer)
    chunk = nextWriteTimer = null
  }
  function closed() {
    clearImmediate(nextWriteTimer)
    socket = null
  }

The test pins down four invariants that Deno already implements but that
the issue reporter expected to behave differently:

1. `clearImmediate(t)` called synchronously cancels `t`.
2. `clearImmediate(t)` called from a microtask before any check phase
   runs cancels `t`.
3. When a `setImmediate` is queued in the same tick that the socket is
   torn down, libuv's check phase (immediates) runs BEFORE the close
   phase. The `'close'` handler's `clearImmediate` therefore observes
   that the immediate has already fired — identical to Node.js.
4. The crash that #34667 hits ("Cannot read properties of null (reading
   'write')") originates from postgres-js's connection pool re-running
   `write()` AFTER `closed()` has nulled `socket`. A `socket && ...`
   guard inside `nextWrite` — the fix in porsager/postgres#1168 — keeps
   the queued callback from dereferencing the nulled socket. The same
   guarded pattern is verified here to ensure Deno never regresses
   against it.

Refs #34667.

Co-Authored-By: Divy Srivastava <me@littledivy.com>
@divybot divybot changed the title test(node): regression test for clearImmediate across socket close test(node): clearImmediate semantics across TCP socket close Jun 4, 2026
@littledivy
littledivy merged commit ec9847e into main Jun 5, 2026
137 checks passed
@littledivy
littledivy deleted the orch/divybot-465 branch June 5, 2026 06:35
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