[TEST] non-draft block-check - close me - #4460
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
…riggerdotdev#3785) - Add ioSerialization utilities for payload serialization - Add api-type.test.ts for new API types - Update trigger payload schema with IOPacket support - Update shared.ts to import IOPacket type - Add changeset for @trigger.dev/core and @trigger.dev/sdk Closes triggerdotdev#3785
🦋 Changeset detectedLatest commit: d95d2bc 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 |
|
Closing automated block-check test PR. |
|
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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
WalkthroughThe pull request adds configurable recovery for runs and sessions replication errors. It adds CLI handling for engine checks, Docker Hub authentication, worker shutdown, and source-map modes. It updates console interception to preserve original console methods. It validates and offloads large trigger payloads through request-specific API clients. It adds integration and unit tests, changesets, and consolidated fix documentation. ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 Warning |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| process.on("SIGINT", signalHandler); | ||
| process.on("SIGTERM", signalHandler); |
There was a problem hiding this comment.
🔴 Ctrl+C during local development still leaves worker processes behind
A cleanup step is registered for interrupt signals (process.on("SIGINT", signalHandler) at packages/cli-v3/src/commands/dev.ts:209) but an earlier global handler already ends the program immediately, so the cleanup never runs and stray worker processes and the lock file remain.
Impact: Pressing Ctrl+C during trigger.dev dev still leaves orphaned worker processes and a stale lockfile behind — exactly the problem this change was meant to fix.
Handler ordering: the global exit handler wins
installExitHandler() is invoked at module load in packages/cli-v3/src/cli/index.ts:46, before program.parseAsync(). It registers process.on("SIGINT", () => process.exit(0)) (packages/cli-v3/src/cli/common.ts:88-95).
Node invokes signal listeners in registration order, so on SIGINT the global handler runs first and calls process.exit(0) synchronously. The dev handler added later (packages/cli-v3/src/commands/dev.ts:201-210) is asynchronous (await cleanup()), so its devInstance.stop() (which kills child workers and removes the lockfile) never executes.
A fix requires either removing/deferring the global exit handler for the dev command, or having the global handler delegate to registered async cleanup routines.
Prompt for agents
The new SIGINT/SIGTERM handlers in devCommand (packages/cli-v3/src/commands/dev.ts) perform async cleanup (devInstance.stop(), which kills child worker processes and removes the lockfile), but they never run to completion because installExitHandler() in packages/cli-v3/src/cli/common.ts registers a SIGINT/SIGTERM listener at CLI startup (packages/cli-v3/src/cli/index.ts:46) that synchronously calls process.exit(0). Node runs listeners in registration order, so the process exits before the dev cleanup awaits anything. Consider either making the global exit handler opt-in/removable (e.g. remove it before starting dev, or have it check whether any async cleanup handlers are registered and defer exit until they resolve), or centralize shutdown so both paths share one handler.
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 installs has no effect
The deployment flow asks the package installer to skip Node engine checks ({ ...options, ignoreEngines: true } at packages/cli-v3/src/commands/deploy.ts:262) but the install step never forwards that request to the package manager, so installs still abort when the project pins a different Node version.
Impact: Deployments on build servers whose Node version doesn't match engines.node continue to fail, and the newly added tests asserting the flags are passed cannot pass.
Flag is parsed but never used at the install call site
ignoreEngines was added to CommonCommandOptions (packages/cli-v3/src/cli/common.ts:17) and picked into UpdateCommandOptions (packages/cli-v3/src/commands/update.ts:21), and deploy.ts:262 passes ignoreEngines: true. However updateTriggerPackages still calls:
await installDependencies({ cwd: projectPath, silent: true });(packages/cli-v3/src/commands/update.ts:261) — no args are built or passed. packages/cli-v3/src/commands/update.test.ts expects installDependencies to be called with args: ["--no-engine-strict"] (npm), ["--config.engine-strict=false"] (pnpm), ["--ignore-engines"] (yarn) and [] otherwise, so the new test suite fails.
Prompt for agents
updateTriggerPackages in packages/cli-v3/src/commands/update.ts accepts an ignoreEngines option (threaded in from deploy.ts) but never uses it: installDependencies({ cwd: projectPath, silent: true }) is called without any package-manager args. Implement the mapping from the detected package manager (already available via detectPackageManager) to the appropriate flag — npm: --no-engine-strict, pnpm: --config.engine-strict=false, yarn: --ignore-engines, empty array otherwise — and pass it as the args option to installDependencies. The expected behaviour is encoded in packages/cli-v3/src/commands/update.test.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this._replicationClient.events.on("error", (error) => { | ||
| this.logger.error("Replication client error", { | ||
| error, | ||
| }); | ||
| this._errorRecovery.handle(error); | ||
| }); |
There was a problem hiding this comment.
🔴 A single bad replication message can restart the replication stream in a loop
Every reported problem from the database change stream now triggers a full reconnect (this._errorRecovery.handle(error) at apps/webapp/app/services/runsReplicationService.server.ts:275), including problems where the stream is still healthy, so a recurring problem can tear the stream down and restart it repeatedly.
Impact: One unparseable change record can put runs/sessions replication into a restart loop, replaying and re-inserting the same data instead of continuing normally.
Not all "error" events mean the stream stopped
The new recovery module assumes "When the LogicalReplicationClient's WAL stream errors ... it calls stop() on itself and stays stopped" (apps/webapp/app/services/replicationErrorRecovery.server.ts:3-5). That is only true for the replication-start query failure (internal-packages/replication/src/client.ts:394-404, which calls stop()).
Two other paths emit error while the connection remains open and the stream keeps delivering data:
- XLogData parse failure:
internal-packages/replication/src/client.ts:359-362(catch inside thecopyDatahandler; no stop). - pg client errors:
internal-packages/replication/src/client.ts:380-382.
In both cases handle() now schedules subscribe(this._latestCommitEndLsn ?? undefined), and subscribe() begins with await this.stop() (internal-packages/replication/src/client.ts:239-241), killing a live stream. Because the resume LSN is the last commit, the same offending WAL record is replayed after the reconnect, producing error → reconnect → replay → error, backing off to at most REPLICATION_RECONNECT_MAX_DELAY_MS but never converging (max attempts defaults to 0 = unlimited). The same wiring exists in apps/webapp/app/services/sessionsReplicationService.server.ts:256.
A safer design would restrict reconnect to errors that actually stopped the client (e.g. check replicationClient.isStopped, exposed at internal-packages/replication/src/client.ts:111-113) or classify fatal vs. transient errors.
Prompt for agents
The new error-recovery wiring in runsReplicationService.server.ts and sessionsReplicationService.server.ts routes every LogicalReplicationClient "error" event into createReplicationErrorRecovery().handle(), which (under the new default "reconnect" strategy) calls subscribe() again. But the client emits "error" in cases where the stream is still alive and was not stopped: XLogData parse failures inside the copyData handler and pg client 'error' events (internal-packages/replication/src/client.ts). Since subscribe() begins by calling stop(), a transient/parse error now tears down a healthy stream, and resuming from the last commit LSN replays the same offending record, which can loop indefinitely (max attempts defaults to unlimited). Consider only triggering recovery when the client is actually stopped (the client exposes an isStopped getter) or distinguishing fatal connection-loss errors from per-message errors before scheduling a reconnect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (loggedInToDockerHub) { | ||
| logger.debug("Logging out from Docker Hub"); | ||
| await x("docker", ["logout"]); | ||
| } |
There was a problem hiding this comment.
🟡 Docker Hub credentials are left logged in when an image build fails
The build sign-out step for Docker Hub only runs on the success path (docker logout at packages/cli-v3/src/deploy/buildImage.ts:700-703), so when the image build fails the machine stays signed in to Docker Hub.
Impact: On shared or long-lived build machines the Docker Hub session persists after a failed build instead of being cleaned up.
Early return on build failure skips the new logout
When buildProcess.exitCode !== 0 the function returns early at packages/cli-v3/src/deploy/buildImage.ts:611-622; that block logs out of cloudRegistryHost but does not check loggedInToDockerHub. The same is true of the earlier failure returns after the Docker Hub login succeeds (e.g. the registry-login failure return). Only the success path at lines 700-703 performs docker logout.
Using a try/finally (or adding the loggedInToDockerHub logout next to each cloudRegistryHost logout) would make cleanup unconditional.
Prompt for agents
In packages/cli-v3/src/deploy/buildImage.ts the new Docker Hub login sets loggedInToDockerHub, but `docker logout` is only executed on the successful return path. All early-return failure paths after the login (registry login failure, build failure at the buildProcess.exitCode check) skip it, leaving credentials stored on the build machine. Restructure so the Docker Hub logout (and ideally the cloud registry logout) runs on every exit path, e.g. via try/finally around the build section.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } else { | ||
| process.stdout.write(body); | ||
| if (severityNumber === SeverityNumber.ERROR) { | ||
| process.stderr.write(body + "\n"); | ||
| } else { | ||
| process.stdout.write(body + "\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 stdio output now appends newlines — a behavior change for consumers parsing raw stdout
The fallback branch changed from process.stdout.write(body) to process.stdout.write(body + "\n"). This is almost certainly the desired behaviour (previously consecutive logs were concatenated on one line), but it changes the byte stream that the dev CLI / log-forwarders read from the child process. Worth confirming nothing downstream relied on the un-terminated writes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| notifyLeaderElectionLost(error) { | ||
| if (isShuttingDown()) return; | ||
| // Only the reconnect strategy should react. For exit, losing the | ||
| // lock to a peer would otherwise trigger a restart loop. For log, | ||
| // we keep historical no-op semantics. | ||
| if (strategy.type !== "reconnect") return; | ||
| scheduleReconnect(error); | ||
| }, |
There was a problem hiding this comment.
🔍 Losing leader election now causes perpetual re-subscribe attempts on every non-leader instance
With the new default strategy (reconnect), a leaderElection(false) event schedules a reconnect with exponential backoff, unlimited attempts by default. In a multi-instance deployment, every follower will now poll for the leader lock roughly every REPLICATION_RECONNECT_MAX_DELAY_MS (60s default) forever, whereas previously followers stayed idle. This is likely the intended takeover behaviour, but it also means each failed attempt logs at error level (logger.error("Replication stream lost — scheduling reconnect")), which will produce steady error-level noise/alerts on healthy follower instances. Consider logging follower lock contention at warn/info.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this._errorRecovery = createReplicationErrorRecovery({ | ||
| strategy: options.errorRecovery ?? { type: "reconnect" }, | ||
| logger: this.logger, | ||
| reconnect: async () => { | ||
| await this._replicationClient.subscribe(this._latestCommitEndLsn ?? undefined); | ||
| }, | ||
| isShuttingDown: () => this._isShuttingDown || this._isShutDownComplete, | ||
| }); |
There was a problem hiding this comment.
🔍 Reconnect does not re-establish the acknowledge interval or flush scheduler if they were torn down
start() in both services does three things: subscribe(), start the acknowledge interval, and start the flush scheduler. The recovery callback only re-runs subscribe(). That is correct for the pure stream-error case (the interval/scheduler keep running), but if recovery ever fires after stop()/teardown() (which clear _acknowledgeInterval), the stream would come back without acknowledgements. stop()/teardown() do not call _errorRecovery.dispose() — only shutdown() does — so a pending reconnect timer scheduled just before stop() would still fire and resubscribe an otherwise stopped service.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function createTriggerPayloadPathPrefix(taskId: string): string { | ||
| const safeTaskId = encodeURIComponent(taskId); | ||
| return `trigger/${safeTaskId}/${Date.now()}-${Math.random().toString(36).slice(2)}/payload`; | ||
| } |
There was a problem hiding this comment.
🔍 SDK-side payload offload writes to a path that is not tied to the resulting run
createTriggerPayloadPathPrefix builds trigger/<taskId>/<timestamp>-<random>/payload, unlike the server-side offload which keys on the run's friendly id (apps/webapp/app/runEngine/concerns/payloads.server.ts:32). Consequences worth confirming: (1) if the trigger request subsequently fails (validation, idempotency conflict, rate limit) the uploaded object is orphaned with no run to attach it to and no cleanup path; (2) objects are no longer discoverable/attributable by run id for retention or debugging. Also note the upload happens before the idempotency check, so repeated triggers with the same idempotency key each re-upload the payload.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Automated block check.