fix(core): retry TASK_MIDDLEWARE_ERROR under the task retry policy - #4449
fix(core): retry TASK_MIDDLEWARE_ERROR under the task retry policy#4449deepshekhardas 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
🦋 Changeset detectedLatest commit: ab651f8 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 |
|
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 (31)
WalkthroughThe changes add configurable reconnect, exit, and log recovery for runs and sessions replication. The CLI adds package engine-check control, Docker Hub login handling, signal-based development cleanup, and centralized source-map configuration. Core updates preserve console interceptor chains and apply retry policies to task middleware errors. Integration and unit tests cover replication recovery, package-manager flags, source-map modes, and retry behavior. Changesets and consolidated release documentation record the fixes. ✨ 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}
There was a problem hiding this comment.
🔍 PR bundles many unrelated changes, contrary to contribution policy
The PR title/description says it only changes TASK_MIDDLEWARE_ERROR retry behaviour (packages/core/src/v3/errors.ts:432-433), but the diff also contains: replication error-recovery for the webapp (new service + env vars + tests), CLI dev signal handling, Docker Hub login in buildImage.ts, source-map toggling, ConsoleInterceptor rework, five unrelated changesets, and a stray consolidated_pr_body.md at the repo root. CONTRIBUTING.md explicitly states only single-issue PRs are accepted, and the stray root markdown file plus the encoding damage in packages/core/src/v3/errors.ts suggest the branch picked up unintended commits/edits. Worth splitting before merging.
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.
🔴 Deployments still fail on strict Node engine checks because the new ignore-engines setting is never used
The new option that is supposed to relax Node version checks is only stored, never handed to the dependency installer (installDependencies({ cwd: projectPath, silent: true }) at packages/cli-v3/src/commands/update.ts:261), so installs still abort on a version mismatch.
Impact: Deployments on build servers with a different Node version keep failing exactly as before, and the new unit tests for this behaviour fail.
How the option is plumbed but dropped
ignoreEngines was added to CommonCommandOptions (packages/cli-v3/src/cli/common.ts:17) and to UpdateCommandOptions (packages/cli-v3/src/commands/update.ts:21), and deployCommand passes ignoreEngines: true (packages/cli-v3/src/commands/deploy.ts:262). However updateTriggerPackages never reads options.ignoreEngines and calls installDependencies with no args. The new test file packages/cli-v3/src/commands/update.test.ts:74-112 asserts args: ["--no-engine-strict"] / ["--config.engine-strict=false"] / ["--ignore-engines"] / [], none of which can pass with the current implementation. No CLI flag (--ignore-engines) is registered on any command either.
Prompt for agents
The ignoreEngines option added to CommonCommandOptions/UpdateCommandOptions is never consumed. In packages/cli-v3/src/commands/update.ts, updateTriggerPackages detects the package manager via detectPackageManager(projectPath) but then calls installDependencies({ cwd: projectPath, silent: true }) without any package-manager-specific engine flags. Implement mapping from the detected package manager name to the correct flag (npm: --no-engine-strict, pnpm: --config.engine-strict=false, yarn: --ignore-engines) and pass it via the args option, using an empty array when ignoreEngines is false, so the new tests in update.test.ts pass and deploy actually ignores engine checks. Also consider registering a user-facing --ignore-engines CLI flag since the schema field currently has no corresponding commander option.
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 during local development still kills the CLI before worker cleanup can run
The new interrupt cleanup step is registered (process.on("SIGINT", signalHandler) at packages/cli-v3/src/commands/dev.ts:209-210) after an earlier global handler that immediately terminates the process, so the cleanup never finishes.
Impact: Pressing Ctrl+C during trigger.dev dev still leaves orphaned worker processes and lockfiles behind.
Handler ordering
installExitHandler() runs at CLI module load (packages/cli-v3/src/cli/index.ts:46) and registers process.on("SIGINT", () => process.exit(0)) plus the same for SIGTERM (packages/cli-v3/src/cli/common.ts:88-95). Node invokes signal listeners in registration order, so that synchronous process.exit(0) runs before the new async signalHandler in devCommand has a chance to await cleanup() (which stops the dev session, the config watcher, and removes the lockfile via startDev's stop()). The finally block cleanup is also skipped because the process exits.
A fix requires either removing/neutralising the global exit handler for the dev command, or having the global handler delegate to a registry of async shutdown hooks.
Prompt for agents
installExitHandler() in packages/cli-v3/src/cli/common.ts registers synchronous SIGINT/SIGTERM listeners that call process.exit(0); it is invoked at CLI startup in packages/cli-v3/src/cli/index.ts. The new async signal handler in devCommand (packages/cli-v3/src/commands/dev.ts) is registered afterwards, so the synchronous exit wins and dev cleanup (devInstance.stop(), watcher stop, lockfile removal) never runs. Rework this so an async shutdown path can complete: e.g. make installExitHandler delegate to a shared list of async shutdown hooks and await them before exiting, or have devCommand remove the default listeners (process.removeAllListeners('SIGINT')) before installing its own.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (this._isShuttingDown) return; | ||
|
|
||
| this._isShuttingDown = true; | ||
| this._errorRecovery.dispose(); |
There was a problem hiding this comment.
🟡 Explicitly stopping a replication service can silently restart it
The auto-recovery timers are only cancelled during graceful shutdown (this._errorRecovery.dispose() at apps/webapp/app/services/runsReplicationService.server.ts:311) and not when the service is stopped or torn down, so a pending retry can re-open the replication stream after it was deliberately stopped.
Impact: Replication can come back to life after being stopped, holding a Postgres replication slot and writing data nobody expects.
Mechanism
createReplicationErrorRecovery suppresses work only when isShuttingDown() returns true, which is wired to _isShuttingDown || _isShutDownComplete (apps/webapp/app/services/runsReplicationService.server.ts:268). Neither stop() (apps/webapp/app/services/runsReplicationService.server.ts:305-313) nor teardown() sets those flags or calls dispose(). Stopping the client tears down the WAL stream, which can itself emit an error event that schedules a reconnect; even without that, an already-scheduled reconnect timer will fire and call this._replicationClient.subscribe(...) again. The same applies to SessionsReplicationService.stop()/teardown() (apps/webapp/app/services/sessionsReplicationService.server.ts:293-317), where _isSubscribed is also left inconsistent after such a resurrection.
Prompt for agents
Error recovery timers created by createReplicationErrorRecovery are only disposed in shutdown() of RunsReplicationService and SessionsReplicationService. stop() and teardown() do not dispose them and do not set any flag that isShuttingDown() observes, so a pending reconnect timer (or an error event emitted while stopping) can re-subscribe the replication client after an explicit stop. Add an explicit 'stopped/disposed' state that both stop() and teardown() set (and that isShuttingDown() consults), and call errorRecovery.dispose() from those methods; make sure start() re-enables recovery so restart-after-stop still works.
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 on the build host when an image build fails
The registry sign-out step at the end of the build (x("docker", ["logout"]) at packages/cli-v3/src/deploy/buildImage.ts:700-703) is skipped on every failure path, so the machine keeps the credentials it was given.
Impact: On shared or CI build machines a failed build leaves the Docker Hub session active instead of cleaning it up.
Failure paths that return early
After a successful docker login (packages/cli-v3/src/deploy/buildImage.ts:479-509), the code returns early on the private-registry login failure branch and on buildProcess.exitCode !== 0 (packages/cli-v3/src/deploy/buildImage.ts:611-622) — the latter logs out of the cloud registry but not Docker Hub. Only the success path reaches the Docker Hub logout. Wrapping the build in try/finally, or mirroring the cloudRegistryHost logout in the failure branch, keeps host state clean.
Prompt for agents
In packages/cli-v3/src/deploy/buildImage.ts, the new Docker Hub login (guarded by DOCKER_USERNAME/DOCKER_PASSWORD) is only paired with a `docker logout` on the success path at the end of the function. Early returns — the private registry login failure branch and the `buildProcess.exitCode !== 0` branch — skip it. Restructure so the logout happens for all exits after a successful login (e.g. try/finally around the build, or add the logout to each failure branch alongside the existing cloudRegistryHost logout).
Was this helpful? React with 👍 or 👎 to provide feedback.
| export class SkipLoggingError extends Error { } | ||
| export class SkipCommandError extends Error { } | ||
| export class OutroCommandError extends SkipCommandError { } |
There was a problem hiding this comment.
🟡 Several changed files are committed with formatting that does not match the project formatter
Multiple files in this change are reformatted away from the repository's Prettier style (for example export class SkipLoggingError extends Error { } at packages/cli-v3/src/cli/common.ts:34-36), which the contributor guide requires before committing.
Impact: The diff contains large unrelated formatting churn and will fail the project's formatting check.
Affected locations
AGENTS.md states: "Formatting is enforced using Prettier. Run pnpm run format before committing." Reformatted-away-from-Prettier code appears in packages/cli-v3/src/cli/common.ts:34-36, throughout packages/cli-v3/src/commands/deploy.ts (template-literal and ternary re-indentation, e.g. packages/cli-v3/src/commands/deploy.ts:504-505, 1287-1292), packages/cli-v3/src/entryPoints/dev-run-worker.ts:128-129,324-330, packages/cli-v3/src/entryPoints/managed-run-worker.ts:300-306, packages/cli-v3/src/deploy/buildImage.ts:589-594, plus the new 4-space-indented files packages/cli-v3/src/utilities/sourceMaps.ts and packages/cli-v3/src/commands/update.test.ts, and packages/core/src/v3/consoleInterceptor.ts:16-26.
| export class SkipLoggingError extends Error { } | |
| export class SkipCommandError extends Error { } | |
| export class OutroCommandError extends SkipCommandError { } | |
| export class SkipLoggingError extends Error {} | |
| export class SkipCommandError extends Error {} | |
| export class OutroCommandError extends SkipCommandError {} |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| stack.push("\n"); | ||
| stack.push(` ❯ ${taskWithIssues.exportName} in ${taskWithIssues.filePath}`); | ||
| stack.push(` ? ${taskWithIssues.exportName} in ${taskWithIssues.filePath}`); |
There was a problem hiding this comment.
🟡 Deployment error output and an error message now contain garbled characters
Non-ASCII characters were replaced with corrupted placeholders in generated output (stack.push(" ? …") at packages/core/src/v3/errors.ts:1158), so users see mangled text instead of the intended symbol.
Impact: Deployment error stacks and one runtime error message display broken characters to users.
Corrupted characters introduced
createTaskMetadataFailedErrorStack previously emitted ❯ ${exportName} in ${filePath} and now emits ? .... Additionally the user-facing ChatChunkTooLargeError message (packages/core/src/v3/errors.ts:647) and several comments (packages/core/src/v3/errors.ts:323,335,750) now contain the U+FFFD replacement character where an em dash used to be, indicating the file was saved with a wrong encoding.
| stack.push(` ? ${taskWithIssues.exportName} in ${taskWithIssues.filePath}`); | |
| stack.push(` ❯ ${taskWithIssues.exportName} in ${taskWithIssues.filePath}`); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (this.sendToStdIO) { | ||
| if (severityNumber === SeverityNumber.ERROR) { | ||
| process.stderr.write(body); | ||
| if (this.originalConsole) { | ||
| switch (severityNumber) { | ||
| case SeverityNumber.INFO: | ||
| this.originalConsole.log(...args); | ||
| break; | ||
| case SeverityNumber.WARN: | ||
| this.originalConsole.warn(...args); | ||
| break; | ||
| case SeverityNumber.ERROR: | ||
| this.originalConsole.error(...args); | ||
| break; | ||
| case SeverityNumber.DEBUG: | ||
| this.originalConsole.debug(...args); | ||
| break; | ||
| default: | ||
| this.originalConsole.log(...args); | ||
| break; | ||
| } | ||
| } 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 console output now goes through the original console methods and gains a trailing newline
The stdio branch previously wrote the formatted body with process.stdout/stderr.write(body) (no newline); it now delegates to the saved console methods with the raw args, and falls back to write(body + "\n") when no interception is active (packages/core/src/v3/consoleInterceptor.ts:95-120). Two consequences worth confirming: (1) log framing changes for anything parsing worker stdout line-by-line — output that previously ran together is now newline-delimited; (2) the saved methods are captured unbound (log: console.log) and invoked as this.originalConsole.log(...), so they run with originalConsole as the receiver. Node's global console methods are bound, but a third-party patcher (the Sentry case this change targets) may install unbound functions that rely on this === console.
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.
🔍 Reconnect strategy treats losing the leader lock as a retryable failure, producing a permanent low-frequency retry loop on non-leader replicas
subscribe() in LogicalReplicationClient emits leaderElection(false) and stops when another instance holds the lock (internal-packages/replication/src/client.ts:255-259). With the default reconnect strategy every non-leader webapp instance will now re-subscribe forever, backing off to REPLICATION_RECONNECT_MAX_DELAY_MS (60s) — a repeated Postgres connect + Redis lock attempt per instance per minute, plus a logger.error("Replication stream lost — scheduling reconnect") each time. That is the intended takeover mechanism, but it means routine multi-instance deployments will emit continuous error-level logs; a lower log level (or a distinct message) for the leader-election path would avoid alert noise.
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.
🔍 Sessions service reconnect bypasses start() and leaves _isSubscribed / ack interval state stale
The recovery hook calls this._replicationClient.subscribe(...) directly (apps/webapp/app/services/sessionsReplicationService.server.ts:246-248) rather than going through start(). If the stream dies and is recovered, _isSubscribed and _acknowledgeInterval are never re-established/updated; in particular after an error that follows stop() (which sets _isSubscribed = false and clears the ack interval) a reconnect would restore the WAL stream without restarting the acknowledge interval, so LSNs would stop being acknowledged and the replication slot would grow. Routing recovery through a method that also restores the ack timer would be safer.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Retries TASK_MIDDLEWARE_ERROR using the task retry policy instead of failing immediately.