fix(webapp): mollifier trigger-time decisions#3773
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 |
|
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 Run ID: 📒 Files selected for processing (34)
WalkthroughThis PR implements a major mollifier (burst buffer) feature for trigger idempotency alongside multiple infrastructure and reliability fixes. The mollifier integration adds gate-based bypass logic, Redis-backed claim coordination, buffer-backed synthetic run synthesis, and claim lifecycle management integrated into the trigger task service and idempotency concern. Supporting changes include environment-driven source map support refactoring across workers, SIGINT/SIGTERM handlers in the dev command, Docker Hub authentication for build images, engine-check bypass for deployments, console interceptor restoration to preserve other interceptor chains, and comprehensive test coverage for all new behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -260,8 +261,7 @@ export async function updateTriggerPackages( | |||
| await installDependencies({ cwd: projectPath, silent: true }); | |||
There was a problem hiding this comment.
🔴 ignoreEngines option accepted but never passed to installDependencies
The ignoreEngines flag is added to CommonCommandOptions and UpdateCommandOptions, and deploy.ts:262 passes { ...options, ignoreEngines: true } to updateTriggerPackages. However, the installDependencies call at packages/cli-v3/src/commands/update.ts:261 is await installDependencies({ cwd: projectPath, silent: true }) — it never reads options.ignoreEngines and never passes engine-ignoring flags (--no-engine-strict for npm, --config.engine-strict=false for pnpm, --ignore-engines for yarn) to the package manager. The tests in packages/cli-v3/src/commands/update.test.ts:74-112 expect these args to be present, so they will fail. The fix described in changeset fix-github-install-node-version-2913.md ("Ignore engine checks during deployment install phase") is completely non-functional — deployments on build servers with Node version mismatches will still fail.
Prompt for agents
The `installDependencies` call at line 261 of `packages/cli-v3/src/commands/update.ts` needs to read `options.ignoreEngines` and pass the appropriate engine-strict-ignoring flag as `args` to `installDependencies`. The flag differs per package manager:
- npm: `--no-engine-strict`
- pnpm: `--config.engine-strict=false`
- yarn: `--ignore-engines`
The `packageManager` variable is already detected at line 254 via `detectPackageManager(projectPath)`. The fix should:
1. Build an `args` array based on `options.ignoreEngines` and the detected `packageManager.name`.
2. Pass `args` to the `installDependencies` call: `await installDependencies({ cwd: projectPath, silent: true, args })`.
The test file `packages/cli-v3/src/commands/update.test.ts` already validates the expected behavior for npm, pnpm, and yarn.
Was this helpful? React with 👍 or 👎 to provide feedback.
| switch (severityNumber) { | ||
| case SeverityNumber.INFO: | ||
| this.originalConsole.log(...args); | ||
| break; |
There was a problem hiding this comment.
🟡 console.info() delegates to originalConsole.log() instead of originalConsole.info()
Both log() and info() methods emit SeverityNumber.INFO, and the switch in #handleLog maps SeverityNumber.INFO to this.originalConsole.log(). This means when the user calls console.info("x"), it's intercepted and forwarded to the original console.log("x") instead of the original console.info("x"). Sentry (the explicit motivation in changeset fix-console-interceptor-2900.md) patches console.info and console.log separately, classifying them as different breadcrumb levels. Routing info through log means Sentry records them at the wrong level, partially undermining the stated fix of "preserving the log chain when other interceptors (like Sentry) are present".
Prompt for agents
The switch statement in `#handleLog` at `packages/core/src/v3/consoleInterceptor.ts` dispatches on `severityNumber`, but both `log()` and `info()` use `SeverityNumber.INFO`. To preserve the console.info vs console.log distinction for downstream interceptors like Sentry, there are two approaches:
1. Use `severityText` (which is "Log" vs "Info") as a secondary discriminator within the `SeverityNumber.INFO` case to choose between `this.originalConsole.log()` and `this.originalConsole.info()`.
2. Use different severity numbers for log vs info (e.g., INFO for info, INFO2 or a custom constant for log).
Approach 1 is simpler and backward-compatible:
case SeverityNumber.INFO:
if (severityText === "Info") {
this.originalConsole.info(...args);
} else {
this.originalConsole.log(...args);
}
break;
This requires threading `severityText` into the `#handleLog` dispatch (it's already a parameter).
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); | ||
| }; |
There was a problem hiding this comment.
🚩 Dev command signal handlers: async cleanup in signal handler may not complete before exit
The signal handler at packages/cli-v3/src/commands/dev.ts:202-206 calls await cleanup() then process.exit(0). Since cleanup() calls devInstance.stop() which may involve async operations (stopping watchers, removing lock files), and process.exit() is called immediately after, there's a question of whether the event loop drains pending microtasks before the process terminates. In Node.js, process.exit() forces immediate exit and does NOT wait for pending I/O. However, the await before process.exit(0) ensures the cleanup() promise resolves first (since the signal handler is async and awaited implicitly). The process.exit(0) only fires after the await completes. This is correct but worth noting: if cleanup() rejects, the unhandled rejection would bypass the process.exit(0) call, leaving the process hanging.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
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.
Key changes
triggerTask.server.ts
Test changes
Fixes #3753