fix(webapp): add overridesBySpanId and environmentId filter for self-hosted OTEL spans (fix #2821) - #4448
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
…hosted OTEL spans (fix triggerdotdev#2821)
🦋 Changeset detectedLatest commit: b0a7682 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 (30)
WalkthroughThe change adds configurable recovery for runs and sessions replication, including reconnect, exit, and log strategies with integration tests. Trace queries now enforce environment scope and trace summaries include span overrides. The CLI adds engine-check controls, Docker Hub authentication, source-map modes, and development worker cleanup. Console interception now preserves original methods for delegation and restoration. Changesets and a consolidated PR description document 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 scope does not match its title/description
The stated purpose is "add overridesBySpanId and environmentId filter for self-hosted OTEL spans", but the diff also contains replication error-recovery for runs/sessions plus new env vars, CLI dev signal handling, source-map toggling, Docker Hub login, engine-strict flags, ConsoleInterceptor rework, five unrelated changesets, and a consolidated_pr_body.md file committed at the repo root. CONTRIBUTING.md explicitly asks for one issue per PR; the mixed scope also makes the changesets and .server-changes entries inconsistent with the actual webapp change (no server-change entry covers the eventRepository/taskEventStore change). consolidated_pr_body.md looks like a leftover artifact that should not be committed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try { | ||
| const devInstance = await startDev({ ...options, cwd: process.cwd(), login: authorization }); | ||
| watcher = devInstance.watcher; | ||
| process.on("SIGINT", signalHandler); | ||
| process.on("SIGTERM", signalHandler); | ||
|
|
||
| devInstance = await startDev({ ...options, cwd: process.cwd(), login: authorization }); | ||
| await devInstance.waitUntilExit(); | ||
| } 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 startup finishes, because the call it waits on returns instantly instead of blocking until the user quits.
Impact: Running the local dev command stops watching and executing tasks right after it starts, so developers cannot run tasks locally.
Mechanism: waitUntilExit is a no-op so the finally-block cleanup runs at startup
startDev returns waitUntilExit = async () => {} (packages/cli-v3/src/commands/dev.ts:312), so await devInstance.waitUntilExit() resolves immediately and control falls into the finally block. Previously the finally only did await watcher?.stop(), which left the dev session (bundler, worker processes, output, MCP server) running. Now cleanup() calls devInstance.stop(), which invokes the DevSessionInstance.stop() (packages/cli-v3/src/dev/devSession.ts:247-254: stops bundling, output and the MCP server), stops the config watcher and calls removeLockFile(). All of this now happens milliseconds after boot rather than on exit.
Prompt for agents
In packages/cli-v3/src/commands/dev.ts, devCommand now calls cleanup() in the finally block, which calls devInstance.stop() and therefore shuts down the whole dev session (bundling, output, MCP server) plus removes the lockfile. Because startDev's waitUntilExit is an empty async function that resolves immediately, this cleanup runs right after startup instead of on exit, killing the dev session. Either make waitUntilExit actually block until the session ends (e.g. resolve on SIGINT/SIGTERM or when the dev session terminates) or keep the finally-block behaviour limited to what it did before (stopping the watcher) and rely on the signal handlers for full cleanup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const signalHandler = async (signal: string) => { | ||
| logger.debug(`Received ${signal}, cleaning up...`); | ||
| await cleanup(); | ||
| process.exit(0); | ||
| }; | ||
|
|
||
| try { | ||
| const devInstance = await startDev({ ...options, cwd: process.cwd(), login: authorization }); | ||
| watcher = devInstance.watcher; | ||
| process.on("SIGINT", signalHandler); | ||
| process.on("SIGTERM", signalHandler); |
There was a problem hiding this comment.
🟡 Interrupting the local dev command still skips the new cleanup step
A global interrupt handler registered when the command-line tool loads terminates the process straight away (process.exit(0) at packages/cli-v3/src/cli/common.ts:89) before the newly added cleanup handler can finish, so worker processes and the lock file may still be left behind.
Impact: Pressing Ctrl+C can still leave stray worker processes running, which is the problem this change intended to fix.
Mechanism: listener registration order
installExitHandler() is invoked at module load in packages/cli-v3/src/cli/index.ts:46, registering a synchronous SIGINT/SIGTERM listener that calls process.exit(0). Node runs signal listeners in registration order, so this listener always runs before the signalHandler registered later inside devCommand (packages/cli-v3/src/commands/dev.ts:209-210). process.exit(0) terminates the process synchronously, so the await cleanup() inside the new async handler never completes.
Prompt for agents
packages/cli-v3/src/cli/index.ts calls installExitHandler() at startup, which registers SIGINT/SIGTERM listeners that immediately call process.exit(0) (packages/cli-v3/src/cli/common.ts:88-95). Because those listeners are registered first, they pre-empt the new async signalHandler added in devCommand, so the cleanup never runs. Consider removing/deferring the global exit handler for the dev command (e.g. process.removeAllListeners('SIGINT') before registering the dev handler, or making installExitHandler cooperate with per-command cleanup hooks).
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 ignore Node version checks during deploy install has no effect
The new setting that is supposed to relax Node version checks while installing packages is passed around but never handed to the installer (installDependencies({ cwd, silent }) in packages/cli-v3/src/commands/update.ts), so deployments still fail when the project pins a stricter Node version.
Impact: The intended fix for build-server deployment failures on Node version mismatches does not actually change anything, and the accompanying new tests fail.
Mechanism: flag threaded through but never applied
ignoreEngines was added to CommonCommandOptions (packages/cli-v3/src/cli/common.ts:16) 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 still calls await installDependencies({ cwd: projectPath, silent: true }) (packages/cli-v3/src/commands/update.ts:260) with no args. No package-manager specific flags (--no-engine-strict, --config.engine-strict=false, --ignore-engines) are ever computed. The new test file packages/cli-v3/src/commands/update.test.ts asserts installDependencies is called with those args, so it will fail. Additionally no CLI option (e.g. --ignore-engines) is registered on any command, so users cannot set it.
Prompt for agents
The ignoreEngines option is now part of CommonCommandOptions/UpdateCommandOptions and deploy.ts passes ignoreEngines: true, but updateTriggerPackages never uses it: installDependencies is still called as installDependencies({ cwd: projectPath, silent: true }). Implement the mapping from the detected package manager (npm -> --no-engine-strict, pnpm -> --config.engine-strict=false, yarn -> --ignore-engines, empty array otherwise) and pass it via the args option, which is what packages/cli-v3/src/commands/update.test.ts expects. Also consider registering a user-facing CLI flag if the option is meant to be settable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| WHERE "traceId" = ${traceId} | ||
| AND "environmentId" = ${environmentId} |
There was a problem hiding this comment.
🟡 Detailed trace view is still not restricted to the requested environment
The detailed trace lookup still fetches spans by trace id only (findDetailedTraceEvents at apps/webapp/app/v3/eventRepository/eventRepository.server.ts:563), even though the environment is known, so the environment restriction added elsewhere in this change is applied inconsistently.
Impact: The detailed trace view can include span data that does not belong to the environment being viewed.
Mechanism
getTraceSummary now passes environmentId into TaskEventStore.findTraceEvents, which filters with AND "environmentId" = ... (apps/webapp/app/v3/taskEventStore.server.ts:188 and :219). The sibling method findDetailedTraceEvents (apps/webapp/app/v3/taskEventStore.server.ts:231-307) still queries only on "traceId", and getTraceDetailedSummary receives environmentId but never forwards it. Both paths back the same self-hosted OTEL span data, so the fix is only half applied.
Prompt for agents
findTraceEvents in apps/webapp/app/v3/taskEventStore.server.ts now takes an environmentId and filters on it, but findDetailedTraceEvents (used by getTraceDetailedSummary, which already receives environmentId) still filters only by traceId. Add the same environmentId parameter and AND "environmentId" = ... predicate to both the partitioned and non-partitioned raw queries in findDetailedTraceEvents and pass environmentId from getTraceDetailedSummary.
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
When an image build fails the process returns without signing out of Docker Hub (return on the build-failure path in packages/cli-v3/src/deploy/buildImage.ts:617-622), unlike the success path which does sign out.
Impact: Stored Docker Hub credentials remain in the local Docker config after a failed build.
Mechanism
The new Docker Hub login sets loggedInToDockerHub = true (packages/cli-v3/src/deploy/buildImage.ts:507) and the logout only happens on the success path (:700-704). The early return when buildProcess.exitCode !== 0 logs out of cloudRegistryHost but not from Docker Hub. The same applies to the other early returns after login (e.g. the registry-login failure path).
Prompt for agents
In packages/cli-v3/src/deploy/buildImage.ts the new Docker Hub login is only undone on the success path. Any early return after the login (notably the build-failure branch that already does `docker logout <cloudRegistryHost>`) leaves the Docker Hub session logged in. Factor the logout into a helper/finally so all return paths after a successful `docker login` also run `docker logout`.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (overrides) { | ||
| const spanOverride: SpanOverride = {}; | ||
| if (overrides.isCancelled) spanOverride.isCancelled = true; | ||
| if (overrides.isError) spanOverride.isError = true; | ||
| if (overrides.duration !== undefined) spanOverride.duration = overrides.duration; | ||
| overridesBySpanId[event.spanId] = spanOverride; | ||
| } |
There was a problem hiding this comment.
🔍 overridesBySpanId omits events, unlike the ClickHouse implementation
The ClickHouse repository stores events in the SpanOverride (apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts:1505-1543), while the Postgres path here only sets isCancelled/isError/duration. This is likely correct rather than a bug: the Postgres getSpan already merges ancestor override events itself in #createSpanFromEvent (apps/webapp/app/v3/eventRepository/eventRepository.server.ts:826-901), so also returning them here would double-add the cancellation/exception events when the UI runs applySpanOverrides (.../runs.$runParam.spans.$spanParam/route.tsx:338-367). Worth a quick confirmation that the span detail panel shows the same events in both stores.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.logger.info("Leader election", { isLeader }); | ||
| if (!isLeader) { | ||
| // Failed leader election doesn't throw or emit an "error" event — | ||
| // subscribe() just emits leaderElection(false), calls stop(), and | ||
| // returns. Route through a dedicated handler so only the reconnect | ||
| // strategy acts; the exit strategy must not restart-loop when | ||
| // another instance holds the lock. | ||
| this._errorRecovery.notifyLeaderElectionLost( | ||
| new Error("Failed to acquire replication leader lock") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔍 Failed leader election now triggers an endless in-process resubscribe loop
subscribe() emits leaderElection(false) and stops whenever another instance already holds the Redlock (internal-packages/replication/src/client.ts:254-259). Routing that into scheduleReconnect means every non-leader webapp instance now retries subscribe forever (backoff capped at REPLICATION_RECONNECT_MAX_DELAY_MS). That is probably the intent (so a follower takes over after a leader dies), but note two consequences: (1) the shared attempt counter is never reset for followers since the start event never fires, so if an operator sets REPLICATION_RECONNECT_MAX_ATTEMPTS > 0, followers permanently give up after N failed elections and can never take over; (2) each attempt performs a Redis lock acquisition and a Postgres connect from every replica.
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.
🔍 stdout behaviour change: newline appended and formatting delegated to console
Two behaviour changes here beyond the delegation fix: the fallback branch now appends "\n" to process.stdout/stderr.write (previously log lines were concatenated with no separator), and when interception is active the raw args are passed to the original console methods rather than the pre-formatted util.format(...args) string. The latter means objects are now formatted by the host console (including any Sentry/other wrapper), which can differ from util.format output and from what is emitted to OTEL. Worth confirming the dev-mode log rendering still parses worker stdout as expected.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Authenticate to Docker Hub if credentials are provided (fixes rate limit issues) | ||
| let loggedInToDockerHub = false; | ||
| if (process.env.DOCKER_USERNAME && process.env.DOCKER_PASSWORD) { | ||
| logger.debug("Logging in to Docker Hub"); | ||
| const loginProcess = x( | ||
| "docker", | ||
| ["login", "--username", process.env.DOCKER_USERNAME, "--password-stdin"], | ||
| { | ||
| nodeOptions: { | ||
| cwd: options.cwd, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| loginProcess.process?.stdin?.write(process.env.DOCKER_PASSWORD); | ||
| loginProcess.process?.stdin?.end(); | ||
|
|
||
| for await (const line of loginProcess) { | ||
| errors.push(line); | ||
| logger.debug(line); | ||
| } | ||
|
|
||
| if (loginProcess.exitCode !== 0) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to login to Docker Hub`, | ||
| logs: extractLogs(errors), | ||
| }; | ||
| } | ||
|
|
||
| loggedInToDockerHub = true; | ||
| options.onLog?.("Successfully logged in to Docker Hub"); | ||
| } |
There was a problem hiding this comment.
🔍 Docker Hub login uses ambient env vars for every local build
The login block triggers whenever DOCKER_USERNAME/DOCKER_PASSWORD are present in the environment, regardless of whether the build actually pulls from Docker Hub, and it runs before the registry-specific login. The login output is pushed into the shared errors array which is later returned as build logs — verify nothing sensitive (e.g. warnings echoing the credential store or username) ends up in surfaced logs. Also note docker logout with no argument only clears the default Docker Hub entry, which matches the login here.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Fixes #2821
Adds overridesBySpanId and environmentId filter support for self-hosted OTEL spans in the webapp.