feat: migrate Prisma from 6.14.0 to 7.7.0 with driver adapters - #4469
feat: migrate Prisma from 6.14.0 to 7.7.0 with driver adapters#4469deepshekhardas wants to merge 20 commits into
Conversation
…t build server failures (triggerdotdev#2913)
- Include reproduction scripts for Sentry (triggerdotdev#2900) and engine strictness (triggerdotdev#2913) - Include PR body drafts for consolidated tracking
- Include reproduction scripts for Sentry (triggerdotdev#2900) and engine strictness (triggerdotdev#2913) - Include PR body drafts for consolidated tracking
When the underlying logical-replication client errored (e.g. after a Postgres failover), the runs and sessions replication services logged the error and left the stream stopped. The host process kept running, the WAL backed up, and ClickHouse silently fell behind. Both services now run a configurable recovery strategy on stream errors, defaulting to in-process reconnect with exponential backoff so a fresh self-hosted setup heals on its own: - "reconnect" (default) re-subscribes via the existing subscribe(lastLsn) path with exponential backoff (1s -> 60s cap, unlimited attempts), which re-validates the publication, re-acquires the leader lock, and resumes from the last acknowledged LSN. - "exit" calls process.exit after a short flush window so a host's supervisor (Docker restart=always, systemd, k8s, etc.) can replace the process. - "log" preserves the historical behaviour. Per-service strategy + exit knobs are env-driven via RUN_REPLICATION_ERROR_STRATEGY / SESSION_REPLICATION_ERROR_STRATEGY plus matching *_EXIT_DELAY_MS / *_EXIT_CODE. Reconnect tuning is shared across both services via REPLICATION_RECONNECT_INITIAL_DELAY_MS / _MAX_DELAY_MS / _MAX_ATTEMPTS (0 = unlimited).
Addresses PR review feedback:
- LogicalReplicationClient.subscribe() can throw before its internal
"error" listener is wired up (notably when pg client.connect() fails
mid-failover). The reconnect strategy's catch block only logged, so
recovery silently stopped. Now also calls scheduleReconnect(err) — the
pendingReconnect guard makes it idempotent if an error event was also
emitted.
- Reject negative values for the new replication-recovery env vars and
cap exit codes at 255.
- Convert the new ReplicationErrorRecovery{Deps,} interfaces to type
aliases to match the repo's TypeScript style.
- Tighten the reconnect dep comment to drop a stale "lastAcknowledgedLsn"
reference (the wrapper-tracked resume LSN is what callers actually pass).
- Restore process.exit after service.shutdown() in the exit-strategy
test so a delayed exit timer can't terminate the test worker.
LogicalReplicationClient.subscribe() can resolve without throwing or emitting an "error" event when leader-lock acquisition fails — it just calls this.stop() and returns. The reconnect callback now checks isStopped after subscribe() and throws so the recovery handler can schedule the next attempt instead of silently giving up.
…rough handle() The previous post-subscribe() isStopped check was always true on the happy path: subscribe() calls stop() up front (setting _isStopped=true) and only resets the flag inside the replicationStart event, which fires asynchronously after subscribe() returns. So the check threw on every successful reconnect, the catch rescheduled, the next attempt tore down the just-built client, and the cycle continued — replication briefly worked between teardowns, which is why the integration test passed. Replace it with the correct nudge: subscribe to leaderElection and call the recovery handler on isLeader=false. That's the only subscribe() exit path that doesn't either throw or emit an "error" event (the other silent-return paths emit "error" first via createPublication/createSlot failures).
The previous commit routed leaderElection(false) through handle(), which under the exit strategy schedules process.exit. In a multi-instance deployment that turns lost leader election — a normal operational state — into a restart loop: exit, supervisor restarts, election fails again, exit, and so on. Add a dedicated notifyLeaderElectionLost() on ReplicationErrorRecovery that the reconnect strategy treats as another retry trigger, while exit and log strategies no-op. Wire the wrapper services through the new method.
fix(webapp): auto-recover replication services after stream errors
…iggerdotdev#3391) - Update schema.prisma for Prisma 7 compatibility - Add prisma.config.ts with driver adapter setup - Update transaction.ts for new Prisma client API - Update docker/entrypoint.sh and Dockerfile for pnpm 10.23.0 - Update package.json across packages for Prisma 7 deps - Update testcontainers, run-engine tests for new Prisma version - Add references/prisma-7/package.json placeholder Closes triggerdotdev#3391
🦋 Changeset detectedLatest commit: 5f4c41a The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (47)
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 |
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } finally { | ||
| await watcher?.stop(); | ||
| process.off("SIGINT", signalHandler); | ||
| process.off("SIGTERM", signalHandler); | ||
| await cleanup(); |
There was a problem hiding this comment.
🔴 Local development mode shuts itself down immediately after starting
The whole local dev session is torn down (await cleanup() at packages/cli-v3/src/commands/dev.ts:217) as soon as start-up finishes, because the call it waits on returns instantly, so running trigger dev kills the task runtime and deletes its lock file right after launch.
Impact: Developers can no longer run tasks locally — the dev session stops working the moment it starts.
Why the wait returns immediately and what cleanup destroys
startDev returns waitUntilExit defined as const waitUntilExit = async () => { }; (packages/cli-v3/src/commands/dev.ts:312), so await devInstance.waitUntilExit() resolves on the next tick and the finally block runs immediately.
Previously the finally only did await watcher?.stop() (stopping the config file watcher), which left the dev session (bundler watch, runtime, MCP server) running. Now cleanup() calls devInstance.stop(), which is startDev's stop: it calls devInstance?.stop() on the DevSessionInstance — destination.remove(), stopBundling(), runtime.shutdown(), stopOutput(), stopMcpServer() (packages/cli-v3/src/dev/devSession.ts:247-255) — plus watcher.stop() and removeLockFile().
The cleanup should only run on actual exit (e.g. from the signal handler), not right after start-up; waitUntilExit needs to return a promise that only resolves when the session actually ends.
Prompt for agents
In packages/cli-v3/src/commands/dev.ts, devCommand now runs `await cleanup()` in a finally block after `await devInstance.waitUntilExit()`. But startDev returns `waitUntilExit` as a no-op async function that resolves immediately, so the finally block executes right after start-up and calls devInstance.stop(), which tears down the DevSessionInstance (destination.remove, stopBundling, runtime.shutdown, stopOutput, stopMcpServer in packages/cli-v3/src/dev/devSession.ts) and removes the lockfile. Before this PR the finally only stopped the config watcher, so the session stayed alive. Fix by making waitUntilExit resolve only when the dev session actually ends (e.g. a promise resolved by the signal handler / session teardown), or by not invoking full cleanup on the normal return path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| process.on("SIGINT", signalHandler); | ||
| process.on("SIGTERM", signalHandler); |
There was a problem hiding this comment.
🟡 Ctrl+C cleanup during local development never runs because an earlier handler exits first
The new interrupt cleanup step is registered after a pre-existing handler that quits the program instantly (process.on("SIGINT", signalHandler) at packages/cli-v3/src/commands/dev.ts:209), so pressing Ctrl+C still terminates before any child processes are cleaned up.
Impact: Orphaned worker processes and stale lock files can still be left behind after interrupting local development.
Handler ordering
installExitHandler() runs at module load in packages/cli-v3/src/cli/index.ts:46 and registers process.on("SIGINT", () => process.exit(0)) (packages/cli-v3/src/cli/common.ts:88-95). Node invokes signal listeners in registration order, and that first listener calls process.exit(0) synchronously, so the async signalHandler added later in devCommand never gets a chance to await cleanup().
A fix would be to make the global exit handler cooperative (e.g. allow commands to register async shutdown steps) or to remove/override the global SIGINT/SIGTERM listeners while the dev command is running.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const UpdateCommandOptions = CommonCommandOptions.pick({ | ||
| logLevel: true, | ||
| skipTelemetry: true, | ||
| ignoreEngines: true, |
There was a problem hiding this comment.
🟡 Option to skip Node version checks during deployment install has no effect
The new setting for skipping Node engine checks is accepted and forwarded (ignoreEngines: true at packages/cli-v3/src/commands/update.ts:21 and packages/cli-v3/src/commands/deploy.ts:262) but the install step never uses it, so deployments still fail when the project declares a stricter Node version.
Impact: The advertised fix for build-server deployment failures on Node version mismatch does not work, and the newly added tests for it fail.
Missing plumbing into installDependencies
updateTriggerPackages still calls await installDependencies({ cwd: projectPath, silent: true }); (packages/cli-v3/src/commands/update.ts:260) with no args. The new test file packages/cli-v3/src/commands/update.test.ts:74-112 asserts installDependencies is called with args: ["--no-engine-strict"] (npm), ["--config.engine-strict=false"] (pnpm), ["--ignore-engines"] (yarn) and [] otherwise — none of which can pass against the current implementation.
The fix is to derive the package-manager-specific flag from options.ignoreEngines and pass it as args to installDependencies.
Prompt for agents
packages/cli-v3/src/commands/update.ts now accepts an `ignoreEngines` option (added to UpdateCommandOptions and passed from deploy.ts), but updateTriggerPackages never uses it: it still calls installDependencies({ cwd, silent: true }) with no args. The new test packages/cli-v3/src/commands/update.test.ts expects package-manager-specific flags to be forwarded (npm: --no-engine-strict, pnpm: --config.engine-strict=false, yarn: --ignore-engines, otherwise []). Implement the mapping using the already-detected packageManager and pass the resulting array as `args` to installDependencies.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "push", | ||
| "--force-reset", | ||
| "--accept-data-loss", | ||
| "--skip-generate", |
There was a problem hiding this comment.
🟡 Container-based tests now regenerate the database client on every run
The flag that prevented client regeneration was dropped from the test database setup command ("--skip-generate" removed at internal-packages/testcontainers/src/utils.ts:32), so every test that starts a Postgres container rewrites the shared generated client directory while other tests are running.
Impact: Test runs get noticeably slower and can fail intermittently when parallel suites regenerate the same generated client files.
Details
createPostgresContainer runs prisma db push --force-reset --accept-data-loss --schema ... --url ... for each container fixture. Without --skip-generate, Prisma regenerates into internal-packages/database/generated/prisma, which is the exact directory already imported by the running test processes. Multiple suites executing container fixtures concurrently write to that directory simultaneously.
Unless Prisma 7 requires generation here (it does not — generation happens at build time), --skip-generate should be restored alongside the new --url argument.
Was this helpful? React with 👍 or 👎 to provide feedback.
| url: process.env.DATABASE_URL ?? "postgresql://localhost:5432/trigger", | ||
| directUrl: process.env.DIRECT_URL, |
There was a problem hiding this comment.
🟡 Database migrations silently fall back to a local database when the connection string is missing
The migration configuration substitutes a hard-coded local database address when no connection string is configured (process.env.DATABASE_URL ?? "postgresql://localhost:5432/trigger" at internal-packages/database/prisma.config.ts:8), so a misconfigured environment no longer fails loudly.
Impact: A deployment with a missing database setting will quietly try to migrate a wrong/local database instead of erroring out.
Behavior change
Previously the datasource block in internal-packages/database/prisma/schema.prisma used env("DATABASE_URL"), and Prisma raised an explicit "environment variable not found" error when it was unset. With the url moved into prisma.config.ts and defaulted, commands like prisma migrate deploy / db push / migrate reset (invoked from docker/scripts/entrypoint.sh) will target postgresql://localhost:5432/trigger instead. db:reset in particular is destructive.
Prefer throwing when DATABASE_URL is absent rather than defaulting.
| url: process.env.DATABASE_URL ?? "postgresql://localhost:5432/trigger", | |
| directUrl: process.env.DIRECT_URL, | |
| url: process.env.DATABASE_URL ?? invariantDatabaseUrl(), | |
| directUrl: process.env.DIRECT_URL, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const adapter = new PrismaPg(url); | ||
| const prisma = new PrismaClient({ adapter }); |
There was a problem hiding this comment.
🔍 Prisma 7 migration missed two remaining datasources client constructions
@trigger.dev/database now generates a Prisma 7 client with engineType = "client" and no url on the datasource, so new PrismaClient({ datasources: { db: { url } } }) no longer works. Two call sites were not migrated to PrismaPg:
internal-packages/testcontainers/src/webapp.ts:204apps/webapp/test/helpers/sharedTestServer.ts:36
Both are test-only helpers (so not reported as bugs), but webapp integration tests that use the containerised webapp fixture will fail at runtime until they are converted the same way internal-packages/testcontainers/src/index.ts was.
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| // Generate deterministic prepared statement names from query SQL so PostgreSQL | ||
| // can reuse cached query plans. Without this, every query uses an anonymous | ||
| // prepared statement that PG must parse and plan from scratch each time. | ||
| statementNameGenerator: (query) => `p_${createHash("sha256").update(query.sql).digest("hex").slice(0, 16)}`, | ||
| } | ||
| ); |
There was a problem hiding this comment.
🔍 Deterministic prepared-statement names rely on SQL text alone
statementNameGenerator: (query) => \p_${sha256(query.sql).slice(0,16)}`derives the prepared-statement name only from the SQL text. node-postgres caches named statements per connection and reuses them when the name matches, so identical SQL is fine. The risk cases are (a) a 64-bit hash collision between two different statements on the same connection (negligible but non-zero), and (b) two logically distinct queries whose SQL text is identical but whose parameter types differ — Postgres would reject the second Parse with "prepared statement already exists" / type mismatch. Worth confirming against@prisma/adapter-pg@7.7.0` semantics (does the adapter guarantee the generator only needs uniqueness per distinct SQL+param-type tuple?) before running this in production, since a failure here surfaces as sporadic query errors rather than a startup failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const coreMetrics = await metricsRegister.metrics(); | ||
|
|
||
| // Order matters, core metrics end with `# EOF`, prisma metrics don't | ||
| const metrics = prismaMetrics + coreMetrics; | ||
|
|
||
| return new Response(metrics, { | ||
| return new Response(coreMetrics, { | ||
| headers: { | ||
| "Content-Type": metricsRegister.contentType, | ||
| }, |
There was a problem hiding this comment.
🔍 Prisma pool/metric observability removed with no replacement
Dropping the metrics preview feature removes $metrics.json() / $metrics.prometheus(), which is why configurePrismaMetrics and the /metrics route composition were deleted. The consequence is that all connection-pool visibility (db.pool.connections.*, db.client.queries.*) disappears from both the OTel meter and the Prometheus scrape endpoint, right as the pool implementation changes from the Rust engine to node-postgres. Since pool sizing/idle behaviour is also being changed in this PR, it would be worth re-adding equivalent gauges from the pg.Pool instance (totalCount, idleCount, waitingCount) so regressions are observable.
(Refers to lines 15-21)
Was this helpful? React with 👍 or 👎 to provide feedback.
| function isDriverAdapterTransactionWriteConflict(error: unknown): boolean { | ||
| if (typeof error !== "object" || error === null) return false; | ||
| const err = error as { name?: string; message?: string }; | ||
| return err.name === "DriverAdapterError" && err.message === "TransactionWriteConflict"; | ||
| } | ||
|
|
||
| export function isPrismaRetriableError(error: unknown): boolean { | ||
| if (isDriverAdapterTransactionWriteConflict(error)) { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔍 Driver-adapter write-conflict detection matches on error message text
isDriverAdapterTransactionWriteConflict matches err.name === "DriverAdapterError" && err.message === "TransactionWriteConflict". This is a string-equality check against an internal Prisma error shape, so any future wording change (or a wrapped/prefixed message) silently disables serialization-failure retries — a failure mode that only shows up as increased 40001 errors under load rather than a test failure. If DriverAdapterError exposes a structured cause/code (e.g. the PG SQLSTATE 40001), matching on that would be more durable. A unit test asserting the retry path against a synthesized error object would also help lock this in.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Use a large debounce so the background processQueueForWorkerQueue job | ||
| // doesn't race with the manual processMasterQueueForEnvironment call. | ||
| // With PrismaPg adapter overhead each trigger() takes longer, so a small | ||
| // debounce causes items to be moved to the worker queue individually in | ||
| // arrival order rather than collectively in priority order. | ||
| processWorkerQueueDebounceMs: 10_000, |
There was a problem hiding this comment.
🔍 Run-engine tests loosened with longer sleeps and a 10s debounce
Several timing assertions were relaxed (200ms → 1000ms sleeps, processWorkerQueueDebounceMs 50 → 10_000, idempotency TTL 200ms → 60s) with the rationale that PrismaPg adds per-query overhead. Two things to keep in mind: (a) the priority test now depends on the background worker-queue job never firing during the test rather than on ordering being correct, so it no longer exercises the debounced path at all; (b) the ~1s sleeps are absolute and will still be flaky on slower CI. If driver-adapter overhead is really large enough to change queue ordering semantics, that itself deserves a measurement before merging — it would affect production throughput too.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Migrates Prisma to 7.7.0 with driver adapters. See original PR #3391.