fix: FT-5226's socket-drop shape kills the canary and could kill a deploy - #211
Merged
Conversation
Prisma Postgres sometimes accepts a cold connection and then drops the socket. `pg` reports that by emitting an `'error'` event on the client, and an `'error'` event with no listener is an uncaught exception, so bun fixed the exit code at 1 while the canary went on to print the correct "bug still present" verdict and delete its project. The version banner bun prints at exit landed just after "Deleting project ...", which reads as a crash during teardown — it is not; teardown succeeded every time. Exit 1 is this canary's instruction to delete `withConnectionRetry` from production code, so a dropped socket was quietly producing that signal on a required check. Each probe client now listens for `'error'` and logs it. Stray uncaught errors and rejections are absorbed so they cannot decide the exit code. Teardown is best-effort, since the CI cleanup job already sweeps the `canary` prefix. A canary that cannot run at all now exits 0 with a warning annotation rather than 1, which was the same false signal. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThe canary now treats only a confirmed 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
commit: |
The entry described only the connect-time rejection, which would lead the next reader to write a retry around the connect alone. The cold window also presents as a completed connection that is then dropped, so the failure lands on the first query — the canary hit both shapes in the same job within two days. Also repoints the workaround source, which named `packages/app-cloud/src/prisma-next-migrate.ts`. That path does not exist; `withConnectionRetry` lives in `packages/1-prisma-cloud/1-extensions/target/src/pg-connection.ts`. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
PgWarm exists to make the first connection to a freshly provisioned database, which makes it the most likely place in the codebase to meet FT-5226's socket-drop shape: the cold upstream accepts the connection and then drops it. `pg` reports that by emitting an `'error'` event on the client, and an `'error'` event with no listener is an uncaught exception. It is raised outside the promise, so `withConnectionRetry` never sees it and the deploy process dies instead of retrying. There is no `uncaughtException` guard anywhere in the deploy path. `warmDatabase` now listens for `'error'` and logs it, matching what the pools in `prisma-next.ts` and `auth-options.ts` already do. It is exported with an optional retry override so the test can drive it without waiting out the default minute of retries. The test needs no real Postgres: a stub speaks enough of the startup protocol for `connect()` to resolve, then drops the socket. Removing the listener makes it fail at `pg/lib/client.js:217`, the emit path. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…l driver Wrapping connect and the operation in `withConnectionRetry` is necessary but not sufficient — every client also needs an `'error'` listener, because an unhandled `'error'` event is raised outside the promise. `@prisma-next/driver-postgres` 0.16.0 has neither, so a socket drop during a deploy-time migration kills the process. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden-electric
changed the title
…d before it Corrects the previous note, which said the unguarded control client needed an upstream fix. It already has one — prisma/prisma 0e51f1f4d, 2026-07-22 — but `@prisma-next/driver-postgres@0.16.0` shipped 2026-07-21 and is the last standalone release, so the fix only reaches us with the Prisma 8 upgrade. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The previous note implied the upgrade was merely large. It is unavailable: `@prisma-next/*` stops at 0.16.0 and npm has no 8.x `prisma` at all, not even a release candidate. Also drops `pnpm patch` from the stopgaps — we are not patching dependencies. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…re adopting My previous two notes were both wrong. The packages were renamed and regrouped: the driver is published as `@prisma/orm-target-postgres@8.0.0-rc.1`, not under `@prisma-next/*` or `prisma`, which is why searching those three scopes found nothing. Verified the listener is in the published tarball at `dist/control-6WFTtLAM.mjs:33`. Composer adopts it in the Prisma 8 upgrade, so no local workaround is needed. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden
approved these changes
Aug 9, 2026
A `data` chunk is typed `string | Buffer`, so `readInt32BE` does not typecheck on it. We never set an encoding, but narrowing with `Buffer.isBuffer` is honest about it and keeps `test:types` green. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FT-5226 has a second shape that isn't in the ticket or in
gotchas.md: instead of rejecting the first connection to a cold database, the proxy accepts it and then drops the socket.pgreports that by emitting an'error'event on the client, and an'error'event with no listener is an uncaught exception. That is raised outside the promise chain, sowithConnectionRetrycannot catch it.This PR fixes the two places that were exposed, and writes the shape down.
1. The canary was failing after it had already succeeded
The "Cold-connect canary (FT-5226)" job intermittently exited 1 straight after printing the verdict that should exit 0. It looked like a crash during project teardown, because the last thing in the log was
Deleting project ...followed by a blank line and bun's version banner.Teardown was never the problem. It succeeded on every observed failure. The uncaught exception happened about a second earlier, during sampling; bun fixes the exit code at 1 at that moment and ignores
process.exitCodefrom then on. The script carried on normally — classified the sample, printed the right verdict, deleted the project — and only at exit did bun print its banner and quit with 1.It reads as a teardown crash because of stream interleaving. The error report goes to stderr and the script's own output to stdout, and GitHub merges them by arrival time. Note where the error text actually sits below:
This was worse than a flake. Exit 1 is this canary's instruction to delete
withConnectionRetryfrom production code, and a dropped socket was producing that signal on a required check.In
scripts/cold-connect-canary.ts:'error', which fixes the cause and prints the socket error instead of hiding it.uncaughtExceptionandunhandledRejectionhandlers log any future stray async error rather than letting it decide the exit code.deleteProjectDeepcall is wrapped, so a failed teardown logs and leaves the verdict alone. The CI cleanup job already sweeps thecanaryprefix.The exit code is assigned once, from the verdict, and is 1 only for
bug-gone.2. The same defect could kill a deploy
PgWarmexists to make the first connection to a freshly provisioned database, which makes it the most likely place in the codebase to meet this. Its client had no'error'listener, and there is nouncaughtExceptionguard anywhere in the deploy path, so a drop would take the deploy process down instead of being retried.warmDatabasenow listens for'error'and logs it, matching what the pools inprisma-next.tsandauth-options.tsalready do. It is exported with an optional retry override so the new test can drive it without waiting out the default minute of retries.3. Written down
The
gotchas.mdFT-5226 entry described only the rejection shape, which would lead the next reader to write a connect-only retry. It now covers the drop, and records that wrapping connect plus the operation is necessary but not sufficient — every client also needs an'error'listener. It also repoints the workaround source, which namedpackages/app-cloud/src/prisma-next-migrate.ts; that path does not exist.Verification
The new test in
pg-warm-resource.test.tsneeds no real Postgres: a stub speaks enough of the startup protocol forconnect()to resolve, then drops the socket. Removing the listener makes it fail atpg/lib/client.js:217, the emit path — so it catches the bug it is there for.For the canary, the same stub reproduced the CI failure on bun 1.3.13 with identical stack frames, the same trailing blank line and banner, and
EXIT=1despiteprocess.exitCode = 0. With the listener attached it exits 0. The setup-failure and missing-credential paths both exit 0 against the live API.Lint is clean. The target package's typecheck errors and 23 test failures are pre-existing — identical with these changes stashed — and this adds one passing test.
Observed in
Known, not fixed here
@prisma-next/driver-postgres@0.16.0builds the control client used for deploy-time migrations around a long-livedpg.Clientwith no'error'listener, so a drop there kills the deploy process rather than failing the migration. Fixed upstream inprisma/prisma@0e51f1f4d, and the fix ships in@prisma/orm-target-postgres@8.0.0-rc.1— verified in the published tarball atdist/control-6WFTtLAM.mjs:33. Composer adopts that in #212, so this needs nothing here beyond thegotchas.mdnote.The sweep loop in
scripts/ci-cleanup.tshas no try/catch, so one transport error abandons every project after it in the list.🤖 Generated with Claude Code