fix: 3753 - #4453
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
Phase 2 of the mollifier rollout — move the divert decision from the drainer (dequeue time) to the call site (trigger time). When the gate returns mollify, the trigger pipeline writes a canonical engine.trigger snapshot into the Redis buffer and returns a synthesised HTTP 200 to the customer immediately. The materialised TaskRun row is created later when the drainer replays the snapshot through engine.trigger. Call-site changes ------------------ - Evaluate the mollifier gate inside the traceRun span instead of before it, so the mollified run gets a ClickHouse PARTIAL event immediately. - Extract #buildEngineTriggerInput() so the same engine.trigger shape is shared between the mollify (buffer snapshot) and pass-through paths. - Replace the Phase-1 dual-write (buffer.accept + engine.trigger) with mollifyTrigger(), which writes the buffer and returns a synthetic result. engine.trigger is no longer called on the mollify path. - Thread idempotency claim ownership through the call pipeline: publish the claim with the synthesised runId on success, release on error. Test changes ------------- - MockTraceEventConcern now produces realistic W3C traceparent values. - mollify dual-write test replaced with synthetic-result + no-PG test. - Removed orphan-entry and debounce-match orphan tests. Drive-by: MollifierBuffer no longer accepts entryTtlSeconds (the sliding-window TTL is now managed internally by the buffer class).
🦋 Changeset detectedLatest commit: f870f30 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 test PR used to verify createPullRequest permission |
|
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 webapp now supports mollifier-based trigger diversion, buffered idempotency resolution, Redis-backed claim coordination, and synthetic run reconstruction. The trigger service publishes or releases claims and shares engine input construction across buffered and pass-through paths. The CLI adds engine-check controls, Docker Hub login handling, signal-based development cleanup, and configurable source-map initialization. Console interception now delegates output to saved console methods. Tests and changesets cover these updates. ✨ 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.
🔍 Large unrelated formatting churn and stray artefacts in the diff
This PR mixes several unrelated fixes (CLI dev cleanup, source maps, Docker Hub auth, engine strictness, console interceptor) with a large webapp mollifier feature, plus wide Prettier-contrary reformatting across packages/cli-v3/src/commands/deploy.ts, dev.ts, update.ts, buildImage.ts and the run-worker entry points (template-literal re-wrapping, { } instead of {}, changed ternary indentation). CONTRIBUTING.md states that only PRs addressing a single issue are accepted and that pnpm run format should be run before committing; the reformatting also makes the substantive changes hard to review. consolidated_pr_body.md at the repo root also looks like an accidentally committed artefact.
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.
🟡 Deployment installs still fail on Node version mismatch because the new ignore-engines setting is never applied
The new ignore-engines setting is accepted and forwarded from the deploy command ({ ...options, ignoreEngines: true } at packages/cli-v3/src/commands/deploy.ts:262) but the install step never reads it, so engine checks still run and deployments keep failing when the project's Node version doesn't match.
Impact: The advertised fix does nothing — builds on machines with a mismatched Node version still abort during dependency installation, and the newly added unit tests for this behaviour fail.
Option plumbed through but never consumed by installDependencies
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 now passes ignoreEngines: true. However updateTriggerPackages still calls await installDependencies({ cwd: projectPath, silent: true }) (packages/cli-v3/src/commands/update.ts:260) with no args. No other code in packages/cli-v3/src reads options.ignoreEngines.
The new test file packages/cli-v3/src/commands/update.test.ts:74-112 asserts that installDependencies is called with args: ["--no-engine-strict"] / ["--config.engine-strict=false"] / ["--ignore-engines"] / [] depending on the package manager — none of which can pass against the current implementation.
Prompt for agents
The deploy command now passes ignoreEngines: true into updateTriggerPackages, and UpdateCommandOptions/CommonCommandOptions declare the flag, but updateTriggerPackages never uses it: the call to installDependencies in packages/cli-v3/src/commands/update.ts still passes only { cwd, silent }. Implement the mapping from options.ignoreEngines plus the detected package manager (npm -> --no-engine-strict, pnpm -> --config.engine-strict=false, yarn -> --ignore-engines, empty otherwise) into the args option of installDependencies, matching the expectations in packages/cli-v3/src/commands/update.test.ts.
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); | ||
|
|
||
| devInstance = await startDev({ ...options, cwd: process.cwd(), login: authorization }); | ||
| await devInstance.waitUntilExit(); |
There was a problem hiding this comment.
🟡 Ctrl+C during local development still kills the CLI before worker processes are cleaned up
The new interrupt cleanup handler is registered after a pre-existing handler that terminates the CLI immediately (process.exit(0) at packages/cli-v3/src/cli/common.ts:89), so pressing Ctrl+C ends the process before the new cleanup can stop child workers and remove the lockfile.
Impact: Orphaned worker processes and stale lockfiles are still left behind after interrupting the dev command, so the intended fix has no effect.
Listener registration order makes the new handler unreachable
installExitHandler() is invoked at CLI module load time (packages/cli-v3/src/cli/index.ts:46), registering process.on("SIGINT", () => process.exit(0)) before any command action runs. devCommand registers its async signalHandler later (packages/cli-v3/src/commands/dev.ts:209-210). Node invokes SIGINT listeners in registration order, so the first listener calls process.exit(0) synchronously and the process terminates before the awaited cleanup() in the second listener can run (its first await never resumes). The finally block that also calls cleanup() never executes either, because process.exit bypasses it.
Prompt for agents
The dev command adds SIGINT/SIGTERM handlers that await cleanup() before exiting, but packages/cli-v3/src/cli/common.ts installExitHandler() already registered SIGINT/SIGTERM listeners that call process.exit(0) synchronously, and it runs at module load in packages/cli-v3/src/cli/index.ts before any command action. Because Node runs listeners in registration order, the process exits before the dev cleanup runs, so worker processes/lockfiles are still orphaned. Consider making the global exit handler cooperative (e.g. have it await registered cleanup callbacks, or skip exiting when a command has installed its own handler) rather than exiting immediately.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (outcome.kind === "timed_out") { | ||
| throw new ServiceValidationError( | ||
| "Idempotency claim resolution timed out", | ||
| 503, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Two simultaneous triggers with the same idempotency key can make the second one fail with a 503 instead of returning the first run
When two triggers share an idempotency key, the second one now blocks waiting for the first (claimOrAwait at apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:271) and is rejected with a 503 error if the first hasn't finished within five seconds, instead of resolving to the existing run as before.
Impact: Callers that fire duplicate idempotent triggers concurrently can receive a hard error for a request that previously succeeded and returned the already-created run.
Wait/timeout path converts a previously benign race into an error response
claimOrAwait polls for up to DEFAULT_CLAIM_WAIT_MS (5s, apps/webapp/app/v3/mollifier/idempotencyClaim.server.ts:17) and returns { kind: "timed_out" } if the winner hasn't published. The concern then throws new ServiceValidationError("Idempotency claim resolution timed out", 503) (apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:328-333).
Before this change the loser proceeded through the pipeline and the Postgres unique constraint produced RunDuplicateIdempotencyKeyError, which RunEngineTriggerTaskService.call retries and resolves into the cached run (apps/webapp/app/runEngine/services/triggerTask.server.ts:600-610). Any winner slower than 5s (slow payload processing, queue-limit checks, Postgres contention), or any winner that fails to publish (publish is best-effort and swallows Redis errors, apps/webapp/app/v3/mollifier/idempotencyClaim.server.ts:179-185), now turns the loser's request into a 503. Falling through to the normal pipeline on timeout would preserve the previous, safe behaviour.
Prompt for agents
In IdempotencyKeyConcern.handleTriggerRequest, a claimOrAwait timeout currently throws ServiceValidationError(503). Previously the same race was resolved harmlessly by the Postgres unique constraint on the idempotency key (RunDuplicateIdempotencyKeyError, retried by RunEngineTriggerTaskService.call, which then returns the cached run). Consider falling through to a normal (claimless) trigger on timeout — the same recovery the code already documents for the 'resolved but unfindable' case — so a slow or non-publishing winner cannot turn a duplicate trigger into a customer-visible 503.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!existingRun && idempotencyKey && !request.body.options?.resumeParentOnCompletion) { | ||
| const buffered = await this.findBufferedRunWithIdempotency( | ||
| request.environment.id, | ||
| request.environment.organizationId, | ||
| request.taskId, | ||
| idempotencyKey, | ||
| ); | ||
| if (buffered) { | ||
| return { isCached: true, run: buffered }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Every idempotency-keyed trigger that misses the database now pays an extra Redis round-trip, even for organisations not enrolled in the new buffering
The buffered-run lookup runs for all organisations (findBufferedRunWithIdempotency at apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:133) without the per-organisation enrolment check that the neighbouring claim step uses, adding a Redis call to the hottest request path for tenants that never buffer anything.
Impact: Trigger latency increases for all idempotency-keyed calls once the feature is switched on globally, even for customers who are not enrolled.
Inconsistent gating between the two buffer touches
The pre-gate claim is explicitly gated by resolveOrgMollifierFlag precisely to keep the Redis round-trip off the hot path for non-enrolled orgs (apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts:251-262, with a long comment explaining the rationale). The buffer fallback added just above it (lines 132-142) performs buffer.lookupIdempotency unconditionally whenever getMollifierBuffer() is non-null, i.e. whenever TRIGGER_MOLLIFIER_ENABLED=1 globally (apps/webapp/app/v3/mollifier/mollifierBuffer.server.ts:29-32). Since a non-enrolled org can never have a buffered run (the gate always returns pass_through for it), this lookup can only ever miss, so the same org-flag guard should apply here.
Prompt for agents
In IdempotencyKeyConcern.handleTriggerRequest, the buffer idempotency fallback (findBufferedRunWithIdempotency) runs for every idempotency-keyed trigger whenever the global mollifier buffer exists, while the pre-gate claim below it is deliberately gated on the per-org mollifier feature flag to avoid a Redis round-trip on the trigger hot path. Non-enrolled orgs can never have buffered runs, so the lookup can only miss. Apply the same per-org flag check (resolveOrgMollifierFlag) to the buffer fallback, ideally resolving the flag once and reusing it for both the fallback and the claim.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const result = await args.buffer.accept({ | ||
| runId: args.runFriendlyId, | ||
| envId: args.environmentId, | ||
| orgId: args.organizationId, | ||
| payload: serialiseMollifierSnapshot(args.engineTriggerInput), | ||
| idempotencyKey: args.idempotencyKey, | ||
| taskIdentifier: args.taskIdentifier, | ||
| }); |
There was a problem hiding this comment.
🔍 Mollifier webapp code depends on buffer APIs and a snapshot module that don't exist in this repo state
The new webapp mollifier code calls a set of MollifierBuffer methods that the shipped @trigger.dev/redis-worker buffer does not implement: claimIdempotency, readClaim, publishClaim, releaseClaim, lookupIdempotency, and an accept() that returns { kind: "accepted" | "duplicate_idempotency" | "duplicate_run_id" }. The real implementation in packages/redis-worker/src/mollifier/buffer.ts:47 has accept(input): Promise<boolean> and no claim APIs, and packages/redis-worker/src/mollifier/index.ts exports no IdempotencyClaimResult / IdempotencyLookupInput types. Additionally, apps/webapp/app/v3/mollifier/mollifierMollify.server.ts:3 and readFallback.server.ts:6 import ./mollifierSnapshot.server, which does not exist in apps/webapp/app/v3/mollifier/.
Even ignoring compilation, the runtime semantics differ: mollifyTrigger branches on result.kind === "duplicate_idempotency", which can never be true against a boolean-returning accept, so an idempotency race in the buffer would silently produce two accepted buffered runs. This PR appears to be missing the corresponding redis-worker half.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "returns divert=false when the sliding window stays under threshold", | ||
| async ({ redisOptions }) => { | ||
| const buffer = new MollifierBuffer({ redisOptions, entryTtlSeconds: 600 }); | ||
| const buffer = new MollifierBuffer({ redisOptions }); |
There was a problem hiding this comment.
🔍 Mollifier buffer tests drop the required entryTtlSeconds option
new MollifierBuffer({ redisOptions }) now omits entryTtlSeconds, but MollifierBufferOptions.entryTtlSeconds is still a required number (packages/redis-worker/src/mollifier/buffer.ts:13,23) and is passed straight into the Lua accept script as String(this.entryTtlSeconds) (line 66). Unless the buffer options are being changed elsewhere in this stack, these tests would construct a buffer whose entry TTL is "undefined". Worth confirming the redis-worker side of the change is intended to make the option optional with a default.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch (error) { | ||
| if (error instanceof RunDuplicateIdempotencyKeyError) { | ||
| //retry calling this function, because this time it will return the idempotent run | ||
| return await this.call({ | ||
| taskId, | ||
| environment, | ||
| body, | ||
| options: { ...options, runFriendlyId }, | ||
| attempt: attempt + 1, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔍 Duplicate-idempotency-key retry can wait on a claim held by the same request
When engine.trigger throws RunDuplicateIdempotencyKeyError, call recurses (apps/webapp/app/runEngine/services/triggerTask.server.ts:601-609) while the outer invocation still holds the pre-gate claim (it is only published/released after the span completes at lines 622-640). The recursive invocation re-enters IdempotencyKeyConcern.handleTriggerRequest; normally its writer-side findFirst sees the winner's row and short-circuits to isCached, so the claim is never contended. But if that lookup misses (e.g. the conflicting row was written and then had its key cleared), the recursion reaches claimOrAwait and polls on the claim its own outer frame owns until the 5s safety net, ending in a 503. Consider threading the already-held claim through the retry so it is not awaited by itself.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
test