Jannik/PaperClipAI
1
0 Fork 0
mirror of https://github.com/paperclipai/paperclip.git synced 2026-10-28 04:02:18 +01:00
Code Issues Packages Projects Releases Wiki Activity
5,734 Commits 1,309 Branches 2,115 Tags
b46d1ab77bbd2628c13c69e0d67f2602bb90dc87
Commit Graph
4 Commits
Author SHA1 Message Date
DottaandPaperclip 4336b9ee02 fix(db): reserve transactions before checking pipeline capacity (#17831) 
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - Interactive requests share a PostgreSQL connection pool with
background work.
> - Prepared queries can queue behind a slow query on a busy connection.
> - Disabling pipelining lets queued work use the next free connection.
> - The driver skips its transaction reservation callback when the
pipeline limit is zero.
> - This pull request runs that callback before the capacity check.
> - Operators can test a pool without pipelining while transactions
retain their connection ownership.

## Linked Issues or Issue Description

**What happened?**

With `PGMAX_PIPELINE=0`, postgres.js 3.4.9 skips `onexecute` in its
short-circuit capacity check. A transaction then lacks its reserved
connection. Real PostgreSQL reproduces `UNSAFE_TRANSACTION` through the
app client and an undefined connection error through the raw driver.
This prevents safe use of the existing setting to reduce query queue
delays.

**Expected behavior**

A transaction must reserve its connection regardless of pipeline
capacity. Commit, rollback, savepoints, and disconnect recovery must
preserve their existing contracts.

**Steps to reproduce**

1. Use the pre-change driver patch from
`e61ee4b005f8e13f64af15f69cb62305dd869f82`.
2. Start disposable PostgreSQL and run the `no-pipeline` scenario in
`packages/db/src/__fixtures__/postgres-connection-recovery.mjs` with
`TEST_DATABASE_URL`, `DRIVER_FORMAT=esm`, and
`RECOVERY_CASE=no-pipeline`.
3. Observe the transaction failure. Apply this patch and repeat with
both `esm` and `cjs`.

Refs #13643, which introduced the existing connection recovery patch.
Searched related open and closed driver PRs. This change extends that
patch and preserves its disconnect safeguards.

## What Changed

- Move the transaction reservation callback before the pipeline capacity
check in both driver distributions.
- Add real PostgreSQL regressions for commits, concurrent transaction
reads, isolation, rollback, savepoints, reserved connections, and
progress on a freed pool connection.
- Document the existing `PGMAX_PIPELINE=0` setting and its driver
prerequisite. Keep the default pipeline limit unchanged.

## Verification

- `pnpm -r typecheck`: passed.
- `pnpm build`: passed.
- Real PostgreSQL 18.6 in an owned, disposable Docker container: the old
patch fails the new transaction scenario in both ESM and CommonJS. The
corrected patch passes all 18 scenarios, including all 12 existing
disconnect recovery cases. The container was removed afterward.
- Local service benchmark: actual `createDb` with prepared statements,
two warmed connections, two controlled advisory lock waits, and the
complete three-query `getDefaultCompanyGoal` path. Two warmups and five
alternating measured samples per setting. Pipeline 100: median 413.44
ms, range 413.33–416.10 ms. Pipeline 0 with the patch: median 12.22 ms,
range 11.86–12.57 ms. Complete SQL, bindings, and results match. Five of
five reads finish while the unrelated slow query remains blocked. These
are synthetic local queue measurements, not hosted latency claims.
- App client compatibility on real PostgreSQL: commit, concurrent
transaction reads, rollback, nested savepoint rollback, dedicated
connections, and liveness connections passed.
- `pnpm test:run`: attempted, then stopped after repeated local embedded
PostgreSQL startup failures caused by the host semaphore limit. This is
an incomplete local full-suite run, not a pass. The focused driver
checks above used disposable Docker PostgreSQL instead. The initial
sandbox attempt also stopped before test execution because tsx could not
bind its IPC socket. Final CI results will be recorded here.
- Current head `72039f71f63530300bf241475d42e857297dfb90`: Greptile and
Paperclip review are both 5/5. All three earlier findings are resolved.
[CI run
40431567516](https://github.com/paperclipai/paperclip/actions/runs/40431567516)
ran the complete validation matrix. The database lane passed all 265
tests, including all 18 real PostgreSQL driver regressions. One cleanup
timer case timed out in server shard 6/12; its isolated real-PostgreSQL
reproduction passed once and failed once, while five runs with the
pre-change driver passed. That evidence is retained and does not
establish the timer failure's cause. A single CI retry of the failed
shard passed. The final aggregate `ci / verify` gate is waiting for a
runner. Its first queued request did not start for more than 14 minutes.
A guarded queue reset confirmed that it was the only unfinished job and
that every test job had passed; attempt 3 restarts only that gate. No
test result was replaced or bypassed. The required gate must pass before
merge.

## Risks

- This changes a shared driver callback order. Both module formats and
existing disconnect recovery are covered by real database tests.
- This PR does not change pool size, enable the setting by default,
retry SQL, or change schemas. Disabling pipelining may reduce batch
throughput. Operators must compare the workload before retaining that
setting.
- The local queue benchmark proves the mechanism. Hosted request
improvement still needs acceptance after the patched image is deployed.

## Model Used

OpenAI GPT-6 in Codex, with code execution and browser developer tools.
The exact serving model identifier and context window are not exposed in
this session.

## 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
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge

---------

Co-authored-by: Paperclip <noreply@paperclip.ing>
 2026-10-27 03:00:01 +00:00
Devin FoleyandPaperclip 685d4faba3 Fix PostgreSQL recovery after a transaction connection closes (#13643) 
Reject queued and late work from disconnected transaction and reservation
scopes. Keep closed reservations out of the open pool, and clear old
connection buffers and responses so new requests can reconnect safely.

Twelve real-PostgreSQL regression cases cover crash prevention, recovery,
and transaction isolation in both ESM and CommonJS. Database checks and
all PR CI checks pass. Greptile: 5/5, no unresolved comments.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
 2026-09-18 16:27:28 -07:00
Devin Foley 8ef39febd7 refactor(db): drop the vendored postgres teardown patch (reverts #12227) (#12234) 
## 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
 2026-08-26 11:10:44 -07:00
Devin Foley 6c7c0fd1f2 fix(db): stop the postgres driver from crashing the process on a write/close race (#12227) 
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - The server and its test suites talk to PostgreSQL through the
`postgres` (postgres.js) driver, and tests routinely tear their
databases down while connections still carry traffic
> - The driver flushes small buffered frames from a `setImmediate`, and
that deferred flush calls `socket.write()` without checking that the
socket still exists; a reserved connection whose backend died keeps
accepting queries, so the flush can fire with a null socket
> - The resulting `TypeError` escapes from a timer callback with no
try/catch above it, crashing the process — in CI this fails suites whose
tests all passed ("Vitest caught 1 unhandled error"), and the same crash
is reported against the driver in the wild after ECONNRESET
> - The latest driver release (3.4.9) still has the bug, so this pull
request adds a pnpm patch guarding the flush and normalizing timer state
on close, plus a deterministic regression test
> - The benefit is CI that no longer fails randomly on a teardown race,
and production processes that survive a database connection dying at the
wrong moment

## Linked Issues or Issue Description

No public issue exists; the underlying problem follows the bug-report
template.

**What happened?**

CI jobs fail with all tests passing: vitest reports `Vitest caught 1
unhandled error during the test run` with `TypeError: Cannot read
properties of null (reading 'write')` at `postgres/src/connection.js`
`Immediate.nextWrite`. The attribution points at whichever test file
happened to be running (e.g. `native-codex-runner.integration.test.ts`),
because the throw comes from a process-level timer callback, not from a
test. The identical crash is reported against the upstream driver by
other projects after `ECONNRESET` (e.g. immich-app/immich#25098).

**Expected behavior**

A connection dying between a write being scheduled and its deferred
flush must settle the affected queries through the driver's normal
connection-error path, never throw from a bare timer callback.

**Steps to reproduce**

Run the new `packages/db/src/postgres-driver-teardown.test.ts` with the
patch removed: reserve a connection (`sql.reserve()` — the same surface
`sql.begin()` uses), destroy the backend socket, wait for the client to
process the close, then issue one query on the reserved connection. The
deferred flush fires one tick later with `socket === null` and crashes
the process with exactly the CI signature.

**Paperclip version or commit**

master `198fc8b28`, `postgres@3.4.9` (latest release; bug still present
on the driver's master branch).

## What Changed

- `patches/postgres@3.4.9.patch` (new, wired via
`pnpm.patchedDependencies`): `nextWrite` returns without writing when
`socket === null`, dropping the buffered bytes — the close path has
already settled every in-flight query, so those bytes have nowhere to
go. The `closed()` and `terminate()` handlers additionally reset
`nextWriteTimer`/`chunk` after `clearImmediate`, so a stale cleared
handle cannot silently block a future reconnect's first flush. All three
shipped builds (`src`, `cjs`, `cf`) get the identical change.
- `packages/db/src/postgres-driver-teardown.test.ts` (new):
deterministic reproduction against a minimal in-process fake wire server
(startup auth + an empty result for the `fetch_types` bootstrap).
Asserts the late query settles with `CONNECTION_DESTROYED` through
`sql.end()` instead of crashing the process.

## Verification

- The regression test fails against unpatched `postgres@3.4.9` with the
exact CI signature (verified by running the same scenario against an
unpatched checkout) and passes with the patch.
- Full `packages/db` suite: 27 files / 101 tests pass.
- Spot-checked server suites that exercise the database through the
patched driver.

## Risks

- Low. The behavioral change activates only in a state that previously
crashed the process (write flush with no socket). Dropping the buffered
bytes matches what the connection's close path already promised callers:
every in-flight query has been settled with a connection error.
- The timer/chunk reset in `closed()`/`terminate()` prevents a
theoretical stale-handle hang after reconnect; on the normal path both
were already reset by `nextWrite`.
- The patch pins to `postgres@3.4.9`; a future driver upgrade will
surface the patch for re-evaluation (pnpm fails loudly on version
mismatch), and the guard can be dropped if the fix lands upstream.

## Model Used

Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code — agentic
coding session with tool use (driver source analysis, wire-protocol fake
server, local test execution).

## 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
 2026-08-26 10:19:19 -07:00