fix(cli-v3): resolve deployment resource exhaustion & flattenAttributes bug #3028
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
🦋 Changeset detectedLatest commit: be79cf2 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. WalkthroughThis pull request consolidates multiple independent bug fixes and enhancements across the CLI, deployment, build, and runtime layers. Changes include: signal handling for graceful shutdown of dev processes and orphaned workers; Docker Hub authentication during builds to handle rate limits; engine strictness control during deployments to support varying Node.js versions; Docker containerfile generation with package manager and lockfile support; source map support refactoring with environment-based configuration; a memory management utility to respawn the CLI with increased heap when needed; improvements to the console interceptor to delegate to original console methods; enhanced error handling for resource-exhausted scenarios; key escaping in attribute flattening utilities; test coverage for new functionality; and dependency version updates. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (34)
✏️ Tip: You can disable this entire section by setting Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 is plumbed through but never passed to installDependencies
The ignoreEngines option is added to CommonCommandOptions, picked into UpdateCommandOptions, and passed as true from deploy.ts:255, but the actual installDependencies call at packages/cli-v3/src/commands/update.ts:261 completely ignores it:
await installDependencies({ cwd: projectPath, silent: true });No args are constructed or passed based on options.ignoreEngines or the detected packageManager.
Root Cause and Impact
The PR description states this fixes "deployment failures on GitHub Integration when engines.node is strict (e.g. "22") by passing --no-engine-strict (and equivalents) during the trigger deploy build phase." The tests in update.test.ts expect installDependencies to be called with args: ["--no-engine-strict"] for npm, args: ["--config.engine-strict=false"] for pnpm, etc. However, the implementation never constructs these args.
The packageManager is detected at line 254 but only used for display messages. The options.ignoreEngines flag is available in scope but never read. This means the entire Engine Strictness fix is non-functional — deployments will still fail when engines.node is strict on the build server.
All four tests in update.test.ts will fail because installDependencies is called without the expected args parameter.
Prompt for agents
In packages/cli-v3/src/commands/update.ts, the installDependencies call at line 261 needs to pass engine-ignoring args based on options.ignoreEngines and the detected packageManager. Replace the installDependencies call around lines 254-261 with logic like:
1. After detecting the packageManager at line 254, compute the args array:
- If options.ignoreEngines is true and packageManager.name is 'npm': args = ['--no-engine-strict']
- If options.ignoreEngines is true and packageManager.name is 'pnpm': args = ['--config.engine-strict=false']
- If options.ignoreEngines is true and packageManager.name is 'yarn': args = ['--ignore-engines']
- Otherwise: args = []
2. Pass these args to installDependencies: await installDependencies({ cwd: projectPath, silent: true, args })
This matches the expectations in update.test.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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; | ||
| } |
There was a problem hiding this comment.
🟡 ConsoleInterceptor routes console.info() calls to originalConsole.log() instead of originalConsole.info()
Both log() and info() use SeverityNumber.INFO as their severity number (consoleInterceptor.ts:72 and consoleInterceptor.ts:76). The new switch statement at line 97-113 only switches on severityNumber, so both map to the SeverityNumber.INFO case which calls this.originalConsole.log(...args). This means console.info() calls are incorrectly dispatched to the original console.log instead of the original console.info.
Impact on Sentry interception
The stated purpose of this change is to fix "ConsoleInterceptor swallowing logs when Sentry (or other monkey-patchers) are present by delegating to the original preserved console methods." If Sentry patches console.info specifically (which it does), those patches will be bypassed because the interceptor dispatches info calls to originalConsole.log instead of originalConsole.info.
The fix should either use severityText to distinguish between "Log" and "Info", or introduce a separate severity number for info vs log.
| 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; | |
| } | |
| switch (severityNumber) { | |
| case SeverityNumber.INFO: | |
| if (severityText === "Info") { | |
| this.originalConsole.info(...args); | |
| } else { | |
| 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; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Consolidated Bug Fixes
This PR combines fixes for several independent issues identified in the codebase, covering CLI stability, deployment/build reliability, and runtime correctness.
Fixes
trigger devleaving orphanedtrigger-dev-run-workerprocesses by ensuring graceful shutdown onSIGINT/SIGTERMand robust process cleanup.ConsoleInterceptorswallowing logs when Sentry (or other monkey-patchers) are present by delegating to the original preserved console methods.engines.nodeis strict (e.g. "22") by passing--no-engine-strict(and equivalents) during thetrigger deploybuild phase.DOCKER_USERNAMEandDOCKER_PASSWORDinbuildImage.tsto authenticate with Docker Hub and avoid rate limits during native builds.TaskRunProcess.execute()by checking specific process connectivity before attempting to send IPC messages.superjsonintopackages/core/src/v3/vendorto resolveERR_REQUIRE_ESMissues in certain environments (Lambda, Node <22.12).onCompleteinuseRealtimehooks when the stream disconnects but the run hasn't actually finished.getRunIdForOptionslogic between SDK and Core to ensure Consistent semantic targets for streams.AnyOnStartAttemptHookFunctionfromtrigger-sdkto allow proper typing ofonStartAttempt.Verification
Automated Verification
packages/cli-v3/src/commands/update.test.ts.repro_2900_sentry.tsscript ensuring logs flow through Sentry patches.Manual Verification
trigger devand observing process cleanup.Changesets
fix-orphaned-workers-2909fix-sentry-console-interceptor-2900fix-github-install-node-version-2913fix-docker-hub-rate-limit-2911fix-dead-process-execute-hangvendor-superjson-esm-fixcalm-hooks-waitconsistent-stream-targetsexport-start-attempt-hook-type