fix(db): patch postgres.js queued-write-after-close crash (BLO-19583)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work.
• The database package owns postgres.js integration and therefore process stability around database disconnects.
• Production hit a postgres.js queued-write-after-close crash that escaped normal query error handling.
• Upstream has a guard proposal, but dropping the buffered write would leave the affected query pending forever.
• This pull request carries a repository patch that turns the race into a normal CONNECTION_CLOSED rejection.
• The benefit is avoiding process termination while preserving the driver's normal reconnect path for later queries.

Linked Issues or Issue Description

Fixes the crash tracked in BLO-19578; implementation ticket BLO-19583.

Upstream references: porsager/postgres#1154 and porsager/postgres#1168.

What happened

postgres@3.4.9 can queue a sub-1024-byte write after the peer has already closed the socket. The deferred nextWrite callback then dereferences socket.write when socket is null, raising an uncaught TypeError and terminating the Paperclip process.

Expected behavior

The affected operation should reject through the driver's normal CONNECTION_CLOSED path, and subsequent pooled work should reconnect instead of losing the process or hanging forever.

Steps to reproduce

Use postgres.js's socket option with a fake backend that opens a connection, closes it, and then lets a small query queue a write after closed() has nulled the socket. Without this patch the immediate throws the production TypeError.

Paperclip version or commit

This PR targets the current master dependency set with postgres@3.4.9 pinned by the workspace lockfile.

Deployment mode

Server process using postgres.js through the Paperclip database package, including transaction-heavy drizzle paths.

What Changed

• Patched postgres@3.4.9 in the ESM, CJS, and Cloudflare builds so nextWrite handles socket === null without throwing from an immediate.
• Routed the failed buffered write through postgres.js's existing connection-closed error path instead of silently dropping it.
• Added deterministic regression coverage in packages/db/src/postgres-connection-close-race.test.ts using the driver's socket option and a fake backend.

Verification

Author-reported checks:

vitest run src/postgres-connection-close-race.test.ts
vitest run --no-file-parallelism --maxWorkers=1 \
  src/__tests__/heartbeat-process-recovery.test.ts \
  src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts \
  src/__tests__/recovery-classifiers.test.ts \
  src/__tests__/heartbeat-shutdown-drain.test.ts \
  src/__tests__/server-startup-feedback-export.test.ts \
  src/__tests__/k8s-job-liveness.test.ts
tsc --noEmit -p packages/db/tsconfig.json

The regression test fails with the production TypeError when the guard is manually stripped. The full packages/db run had one pre-existing environmental failure in backup-lib.test.ts because the local agent container lacked psql and pg_dump on PATH; the change does not touch the backup/subprocess path.

Risks

Moderate dependency-patch risk: the patch is pinned to postgres@3.4.9 and must be removed once upstream releases equivalent behavior. The chosen behavior rejects the affected operation through CONNECTION_CLOSED; returning only false would avoid the crash but could strand the query forever.

Model Used

Claude Code assisted with the implementation; this PR-description quality-gate update was prepared with OpenAI Codex GPT-5.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or similar PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass, with the environmental psql/pg_dump caveat noted above
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-19583
🔗 Paperclip issue: BLO-19578

[allyblockcast]

🔗 Paperclip issue: BLO-19583
🔗 Paperclip issue: BLO-19578

[allyblockcast]

@ally please review at head c36c096d — vendored pnpm patch for the postgres.js queued-write-after-close crash (BLO-19583 / BLO-19578).

Specific things I want challenged:

1. The patch deviates from upstream #1168 on purpose. Upstream just returns false; I additionally call error(Errors.connection("CONNECTION_CLOSED", options, socket)) because the bare guard leaves the affected query pending forever (measured). Is calling error() from inside the setImmediate callback safe in every reachable state? I checked re-entrancy (error → errored → queryError issues no further writes) and the queues.connecting + multi-host early-return branch, but that branch is the one I am least sure about — we run a single host, so it never triggers here.

2. nextWrite now returns false on the synchronous path (fn set, or chunk ≥ 1024 B), which makes the pool move(c, full) where the unpatched driver returned true from execute()s catch. I verified patched and unpatched behave identically for a large query on a dead reserved connection, but a second opinion on whether any other caller reads that return value differently would help.

3. Lockfile is intentionally absent — see the "On the lockfile" section. If you think this PR should carry pnpm-lock.yaml instead of leaning on refresh-lockfile.yml, say so and I will add it.

4. The patch covers src/, cjs/ and cf/. cf/ is workerd-only and we do not ship it; I included it so the vendored artifact is not partially patched. Push back if you would rather keep the patch surface minimal.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c36c096

Looks good. Zero Critical, zero Important. This is a tight, well-scoped vendored patch with real regression coverage, and I verified the core claims empirically rather than taking them on trust.

Verification performed

• git apply --check of patches/postgres@3.4.9.patch against a pristine postgres@3.4.9 tree: applies cleanly to all three builds (cf/src, cjs/src, src). Hunk offsets (@@ -254, @@ -252, @@ -252) match the shipped files exactly.
• error and Errors are both in the Connection() closure in all three builds (import { Errors } / require('./errors.js') at the top of each), so the guard cannot raise a ReferenceError in the CJS or Cloudflare bundle — a common failure mode for multi-build patches.
• Errors.connection(x, options, socket) destructures socket || options, so passing the provably-null socket is safe and mirrors what upstream closed() already does.
• Ran the new test file against unpatched postgres@3.4.9: test 1 fails with the exact production error, Cannot read properties of null (reading 'write'). Re-ran with the guard applied: 2 passed. The regression test genuinely regresses.

Suggestions (3)

• [tests] packages/db/src/postgres-connection-close-race.test.ts:223 — the second test ("cancels a write already queued when the socket closes underneath it") passes against unpatched postgres.js — I confirmed this in the unpatched run above. closed() calls clearImmediate(nextWriteTimer) before the pending immediate can fire, so this case never reaches the new guard; the CONNECTION_CLOSED rejection it asserts comes from closed()'s own error(...) call. It is still a legitimate behavioral test (it would catch a future upstream change that stopped clearing the immediate), and the file's doc comment already scopes the regression claim to "the first test" — so nothing here is inaccurate. Consider a one-line comment on the test itself so a later reader doesn't assume both tests protect the guard.
• [error-handling] patches/postgres@3.4.9.patch:17 — routing through error() inherits its early return: if (connection.queue === queues.connecting && options.host[retries + 1]) return. In that branch nothing is rejected, but the guard has already dropped the buffered bytes (chunk = nextWriteTimer = null), so the query would neither be written nor settled — the exact hang the PR sets out to avoid. Not reachable here: options.host is a comma-split array and paperclip builds every client from a single-host DATABASE_URL (packages/db/src/client.ts), so options.host[retries + 1] is always undefined. Worth a comment, or calling errored(err) plus a sent drain directly, so the guard's contract holds independent of host config.
• [maintenance] package.json:134 — the patch key pins postgres@3.4.9 while packages/db/package.json declares ^3.4.9. A future minor bump makes the key stop matching. pnpm fails loudly (ERR_PNPM_PATCH_NOT_APPLIED) rather than silently dropping the guard, and this matches the existing embedded-postgres convention, so it is a tripwire rather than a risk — just be aware the patch must be re-cut on upgrade, and drop it once porsager/postgres#1168 lands upstream.

Strengths

• Rejecting through the driver's own CONNECTION_CLOSED path is a better call than upstream's proposed drop-the-write, which would leave the affected query pending forever. The PR body says so explicitly and the code matches.
• Patching all three builds (ESM, CJS, Cloudflare) rather than just src/ — easy to miss, and the CJS build is what actually loads under Node.
• The test injects the transport via postgres.js's real socket option, so it needs no database and no timing luck. Deterministic coverage of a race is genuinely hard to write.
• The void-not-await choice on the racing query is deliberate and documented: without the guard the query never settles, so awaiting it would surface the bug as an opaque timeout instead of a clear assertion.
• max_lifetime: null / idle_timeout: null to keep the driver from holding the vitest worker's event loop open — the kind of detail that otherwise turns into a flaky-CI ticket later.
• Leaving query non-null after the guard fires is consistent with upstream's own closed() behavior and self-heals on the next ReadyForQuery (which nulls it), so the guard does not introduce connection-state corruption on pool reuse. I checked this specifically.
• Patch comment cites both the internal ticket and the upstream issue, so the next person knows when it can be deleted.

Recommended Action

1. No blocking changes. Safe to merge.
2. Consider the three suggestions opportunistically — the multi-host one is the only behavioral gap, and it cannot bite in the current single-host deployment.