From 8ef39febd7132de5efb5d61549bd7d2c4b12a1b2 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 26 Aug 2026 11:10:44 -0700 Subject: [PATCH] refactor(db): drop the vendored postgres teardown patch (reverts #12227) (#12234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - #12227 vendored a pnpm patch of the `postgres` driver to stop a teardown race (`nextWrite` firing after the socket is nulled) from crashing the process and failing green CI shards > - Maintainer call: carrying a vendored driver patch is not worth it for a CI flake — the patch adds a maintenance obligation on every future driver upgrade > - The race is an upstream bug in `postgres@3.4.9`; the plan is to wait for an upstream release that fixes it and bump the dependency instead > - This pull request reverts #12227 in full: the patch file, its `package.json` registration, and the regression test that exercised the patched behavior > - The benefit is an unmodified dependency graph; the known flake signature returns and is retried when it bites ## Linked Issues or Issue Description Reverts #12227. **What existing behavior does this improve?** Dependency hygiene: `postgres@3.4.9` is consumed unmodified again, with no `pnpm.patchedDependencies` entry to re-evaluate on every driver upgrade. **Current behavior** The repo carries `patches/postgres@3.4.9.patch` (null-socket guard in the driver's deferred write flush, plus an `execute()` refusal on socketless connections) and a regression test for it. **Proposed behavior** Plain upstream `postgres@3.4.9`. The teardown race stays an upstream bug: a green test shard can occasionally fail with `Vitest caught 1 unhandled error` and `TypeError: Cannot read properties of null (reading 'write')` at `Immediate.nextWrite`; the remedy is retrying the shard until an upstream driver release fixes the race and we bump. **Reason and benefit** A vendored driver patch is a standing maintenance cost that outweighs the flake it suppressed. ## What Changed - Reverts #12227 (`6c7c0fd1f`) in full: removes `patches/postgres@3.4.9.patch`, its `pnpm.patchedDependencies` registration in root `package.json`, and `packages/db/src/postgres-driver-teardown.test.ts`. No lockfile involvement — the merged commit never touched `pnpm-lock.yaml` and the refresh bot had not yet recorded the patch. ## Verification - `pnpm install` on the reverted tree is coherent; the full `packages/db` suite passes (26 files / 100 tests). - `git revert` applied cleanly with no conflicts. ## Risks - Low. This restores the exact pre-#12227 state. The known flake signature returns; it fails jobs whose tests all passed and is cleared by retrying the shard. ## Model Used Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code — agentic coding session with tool use. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] 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 - [x] I will address all Greptile and reviewer comments before requesting merge --- package.json | 3 +- .../db/src/postgres-driver-teardown.test.ts | 108 ------------ patches/postgres@3.4.9.patch | 156 ------------------ 3 files changed, 1 insertion(+), 266 deletions(-) delete mode 100644 packages/db/src/postgres-driver-teardown.test.ts delete mode 100644 patches/postgres@3.4.9.patch diff --git a/package.json b/package.json index 6fc29e7f72..9dfde2719f 100644 --- a/package.json +++ b/package.json @@ -83,8 +83,7 @@ "pnpm": { "patchedDependencies": { "embedded-postgres@18.1.0-beta.16": "patches/embedded-postgres@18.1.0-beta.16.patch", - "acpx@0.12.0": "patches/acpx@0.12.0.patch", - "postgres@3.4.9": "patches/postgres@3.4.9.patch" + "acpx@0.12.0": "patches/acpx@0.12.0.patch" }, "overrides": { "rollup": ">=4.59.0", diff --git a/packages/db/src/postgres-driver-teardown.test.ts b/packages/db/src/postgres-driver-teardown.test.ts deleted file mode 100644 index 55b7ff04a9..0000000000 --- a/packages/db/src/postgres-driver-teardown.test.ts +++ /dev/null @@ -1,108 +0,0 @@ -import net from "node:net"; -import { afterEach, describe, expect, it } from "vitest"; -import postgres from "postgres"; - -/** - * Regression coverage for patches/postgres@3.4.9.patch. - * - * The unpatched driver crashes the whole process when a write races a - * connection close: `write()` buffers small frames and flushes them from a - * `setImmediate(nextWrite)`, and `nextWrite` called `socket.write()` without - * checking that the socket still exists. A reserved connection (sql.reserve / - * sql.begin) whose backend dies keeps accepting queries — its handler calls - * `execute()` with no socket-state check — so the deferred flush fired with - * `socket === null` and threw `TypeError: Cannot read properties of null - * (reading 'write')` from a timer callback with no try/catch above it. In CI - * that surfaced as vitest's "Vitest caught 1 unhandled error" failing suites - * whose tests had all passed, whenever a test tore down its database while a - * connection still had traffic. - * - * The fake server below speaks just enough wire protocol to hand the client - * an open connection (startup auth, then an empty result set for the - * fetch_types bootstrap query); the crash path itself never needs a real - * query to succeed. Against the unpatched driver this test fails through - * vitest's unhandled-error detection with exactly the CI signature; with the - * patch, the late query settles through the normal CONNECTION_DESTROYED path. - */ -describe("postgres driver teardown race", () => { - let server: net.Server | null = null; - let sql: ReturnType | null = null; - - afterEach(async () => { - if (sql) await sql.end({ timeout: 1 }).catch(() => {}); - sql = null; - if (server) await new Promise((resolve) => server!.close(resolve)); - server = null; - }); - - it("does not crash the process when a reserved connection's backend dies before a write flushes", async () => { - const backendSockets: net.Socket[] = []; - - // AuthenticationOk (R, code 0) + ReadyForQuery (Z, idle). - const authOk = Buffer.from([0x52, 0, 0, 0, 8, 0, 0, 0, 0]); - const readyForQuery = Buffer.from([0x5a, 0, 0, 0, 5, 0x49]); - const emptyQueryReply = Buffer.concat([ - Buffer.from([0x31, 0, 0, 0, 4]), // ParseComplete - Buffer.from([0x32, 0, 0, 0, 4]), // BindComplete - Buffer.from([0x54, 0, 0, 0, 6, 0, 0]), // RowDescription, zero fields - // CommandComplete "SELECT 0" - Buffer.from([0x43, 0, 0, 0, 0x0d, 0x53, 0x45, 0x4c, 0x45, 0x43, 0x54, 0x20, 0x30, 0]), - ]); - - server = net.createServer((socket) => { - backendSockets.push(socket); - let greeted = false; - socket.on("data", () => { - if (!greeted) { - // First frame is the StartupMessage. - greeted = true; - socket.write(Buffer.concat([authOk, readyForQuery])); - return; - } - // Any later frame (the fetch_types bootstrap query) gets an empty - // result set + ReadyForQuery, enough for the client to finish - // opening and resolve the reserve. - socket.write(Buffer.concat([emptyQueryReply, readyForQuery])); - }); - socket.on("error", () => {}); - }); - await new Promise((resolve) => server!.listen(0, "127.0.0.1", resolve)); - const port = (server.address() as net.AddressInfo).port; - - const closed = new Promise((resolve) => { - sql = postgres({ - host: "127.0.0.1", - port, - user: "test", - database: "test", - max: 1, - idle_timeout: 0, - connect_timeout: 5, - onclose: () => resolve(), - }); - }); - - // A reserved connection keeps routing queries straight to this one - // physical connection — the same surface sql.begin() transactions use. - const reserved = await sql!.reserve(); - - // Kill the backend and wait until the client has fully processed the - // close (socket nulled, pool notified). - for (const socket of backendSockets) socket.destroy(); - await closed; - - // Late query on the dead reserved connection: unpatched, this buffered - // its frame and scheduled the deferred flush that crashed the process - // one tick later. Patched, execute() refuses it up front, so it settles - // immediately — no pool shutdown required. - const settled = await reserved`select 1`.catch((error: unknown) => error); - expect(settled).toBeInstanceOf(Error); - expect(String((settled as Error).message)).toContain("CONNECTION_CLOSED"); - - // Let any stray deferred flush run before the test ends, so a regression - // in the nextWrite guard still fails this test via the unhandled error. - await new Promise((resolve) => setImmediate(() => setImmediate(resolve))); - - reserved.release(); - }); -}); diff --git a/patches/postgres@3.4.9.patch b/patches/postgres@3.4.9.patch deleted file mode 100644 index bc47aee9c7..0000000000 --- a/patches/postgres@3.4.9.patch +++ /dev/null @@ -1,156 +0,0 @@ -diff --git a/cf/src/connection.js b/cf/src/connection.js -index 8e79170aeb13efe8c3c77ccede0cd3a115b5e1b5..c17a54b62fefa1f13bc38cd82e25d52ed467c94e 100644 ---- a/cf/src/connection.js -+++ b/cf/src/connection.js -@@ -159,6 +159,14 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - if (terminated) - return queryError(q, Errors.connection('CONNECTION_DESTROYED', options)) - -+ // The connection lost its socket (backend death, network close) and has -+ // not reconnected yet. Reserved connections keep routing queries here, -+ // and buffering their frames would schedule a flush with nowhere to -+ // write. Refuse up front the same way `terminated` does, so the query -+ // settles immediately instead of dangling until pool shutdown. -+ if (socket === null) -+ return queryError(q, Errors.connection('CONNECTION_CLOSED', options, socket)) -+ - if (stream) - return queryError(q, Errors.generic('COPY_IN_PROGRESS', 'You cannot execute queries during copy')) - -@@ -254,6 +262,16 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - } - - function nextWrite(fn) { -+ if (socket === null) { -+ // The connection closed between this write being scheduled and the -+ // immediate firing (or a caller raced the close handler). The close -+ // path has already settled every in-flight query, so the buffered -+ // bytes have nowhere to go: drop them instead of crashing the -+ // process from a timer callback with no try/catch above it. -+ nextWriteTimer !== null && clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null -+ return false -+ } - const x = socket.write(chunk, fn) - nextWriteTimer !== null && clearImmediate(nextWriteTimer) - chunk = nextWriteTimer = null -@@ -427,6 +445,7 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - error(Errors.connection('CONNECTION_DESTROYED', options)) - - clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null - if (socket) { - socket.removeListener('data', data) - socket.removeListener('connect', connected) -@@ -440,6 +459,7 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - remaining = 0 - incomings = null - clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null - socket.removeListener('data', data) - socket.removeListener('connect', connected) - idleTimer.cancel() -diff --git a/cjs/src/connection.js b/cjs/src/connection.js -index 07f6716702ac888215c86ad6e0071aaf55ae519c..949aa427e1afd0f68bc4cdcdf062e571568fd24d 100644 ---- a/cjs/src/connection.js -+++ b/cjs/src/connection.js -@@ -157,6 +157,14 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - if (terminated) - return queryError(q, Errors.connection('CONNECTION_DESTROYED', options)) - -+ // The connection lost its socket (backend death, network close) and has -+ // not reconnected yet. Reserved connections keep routing queries here, -+ // and buffering their frames would schedule a flush with nowhere to -+ // write. Refuse up front the same way `terminated` does, so the query -+ // settles immediately instead of dangling until pool shutdown. -+ if (socket === null) -+ return queryError(q, Errors.connection('CONNECTION_CLOSED', options, socket)) -+ - if (stream) - return queryError(q, Errors.generic('COPY_IN_PROGRESS', 'You cannot execute queries during copy')) - -@@ -252,6 +260,16 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - } - - function nextWrite(fn) { -+ if (socket === null) { -+ // The connection closed between this write being scheduled and the -+ // immediate firing (or a caller raced the close handler). The close -+ // path has already settled every in-flight query, so the buffered -+ // bytes have nowhere to go: drop them instead of crashing the -+ // process from a timer callback with no try/catch above it. -+ nextWriteTimer !== null && clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null -+ return false -+ } - const x = socket.write(chunk, fn) - nextWriteTimer !== null && clearImmediate(nextWriteTimer) - chunk = nextWriteTimer = null -@@ -425,6 +443,7 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - error(Errors.connection('CONNECTION_DESTROYED', options)) - - clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null - if (socket) { - socket.removeListener('data', data) - socket.removeListener('connect', connected) -@@ -438,6 +457,7 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - remaining = 0 - incomings = null - clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null - socket.removeListener('data', data) - socket.removeListener('connect', connected) - idleTimer.cancel() -diff --git a/src/connection.js b/src/connection.js -index 1b1cccde43b4570d5d071d6ffaab2669b3c2065a..8bb205e5b4b92282711ec8d105960508ccfc4252 100644 ---- a/src/connection.js -+++ b/src/connection.js -@@ -157,6 +157,14 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - if (terminated) - return queryError(q, Errors.connection('CONNECTION_DESTROYED', options)) - -+ // The connection lost its socket (backend death, network close) and has -+ // not reconnected yet. Reserved connections keep routing queries here, -+ // and buffering their frames would schedule a flush with nowhere to -+ // write. Refuse up front the same way `terminated` does, so the query -+ // settles immediately instead of dangling until pool shutdown. -+ if (socket === null) -+ return queryError(q, Errors.connection('CONNECTION_CLOSED', options, socket)) -+ - if (stream) - return queryError(q, Errors.generic('COPY_IN_PROGRESS', 'You cannot execute queries during copy')) - -@@ -252,6 +260,16 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - } - - function nextWrite(fn) { -+ if (socket === null) { -+ // The connection closed between this write being scheduled and the -+ // immediate firing (or a caller raced the close handler). The close -+ // path has already settled every in-flight query, so the buffered -+ // bytes have nowhere to go: drop them instead of crashing the -+ // process from a timer callback with no try/catch above it. -+ nextWriteTimer !== null && clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null -+ return false -+ } - const x = socket.write(chunk, fn) - nextWriteTimer !== null && clearImmediate(nextWriteTimer) - chunk = nextWriteTimer = null -@@ -425,6 +443,7 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - error(Errors.connection('CONNECTION_DESTROYED', options)) - - clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null - if (socket) { - socket.removeListener('data', data) - socket.removeListener('connect', connected) -@@ -438,6 +457,7 @@ function Connection(options, queues = {}, { onopen = noop, onend = noop, onclose - remaining = 0 - incomings = null - clearImmediate(nextWriteTimer) -+ chunk = nextWriteTimer = null - socket.removeListener('data', data) - socket.removeListener('connect', connected) - idleTimer.cancel()