fix(cli-v3): ensure worker cleanup on SIGINT/SIGTERM (#2909) #2993
Conversation
…t build server failures (triggerdotdev#2913)
🦋 Changeset detectedLatest commit: c97cbcc 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 |
WalkthroughThis pull request adds multiple bugfixes and test/artifact files: signal handling in the CLI dev command to cleanly stop worker processes; optional Docker Hub login/logout during local image builds when DOCKER_USERNAME/DOCKER_PASSWORD are set; an ignoreEngines option for package updates with package-manager-specific install flags and accompanying unit tests; ConsoleInterceptor now stores and delegates to the original console methods (log/info/warn/error/debug) and restores them after interception; several changeset entries and repro/example scripts were added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
🧠 Learnings (9)📚 Learning: 2026-01-15T11:50:06.067ZApplied to files:
📚 Learning: 2025-11-27T16:27:35.304ZApplied to files:
📚 Learning: 2025-11-26T14:40:07.146ZApplied to files:
📚 Learning: 2025-11-27T16:26:44.496ZApplied to files:
📚 Learning: 2026-01-15T10:48:02.687ZApplied to files:
📚 Learning: 2025-11-27T16:26:37.432ZApplied to files:
📚 Learning: 2026-01-15T11:50:06.067ZApplied to files:
📚 Learning: 2025-11-27T16:27:35.304ZApplied to files:
📚 Learning: 2025-10-08T11:48:12.327ZApplied to files:
🧬 Code graph analysis (2)repro-sentry/repro_2900_sentry_cjs_v2.js (1)
repro_2900_sentry.ts (2)
🔇 Additional comments (12)
✏️ Tip: You can disable this entire section by setting 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}
| devInstance = await bootDevSession(watcher.config); | ||
|
|
||
| const waitUntilExit = async () => {}; | ||
| const waitUntilExit = async () => { }; |
There was a problem hiding this comment.
🔴 waitUntilExit is an empty function that immediately resolves, causing dev command to exit immediately
The waitUntilExit function is defined as an empty async function that immediately resolves.
Click to expand
Bug Mechanism
At packages/cli-v3/src/commands/dev.ts:291, the function is defined as:
const waitUntilExit = async () => { };This function returns a Promise that immediately resolves to undefined. When called at line 201:
await devInstance.waitUntilExit();The await completes instantly, then the finally block at lines 202-206 executes, which calls cleanup() that stops the dev instance.
Expected vs Actual Behavior
- Expected: The
waitUntilExitshould block/wait indefinitely until the dev session is terminated by a signal or user action. - Actual: The function returns immediately, causing the dev command to start and then immediately stop itself.
Impact
The dev command (trigger.dev dev) will start up the worker runtime and then immediately shut it down, making the dev command unusable. Users will see the dev session start briefly and then exit.
Recommendation: The waitUntilExit function should return a Promise that never resolves (or resolves only when signaled to exit). For example:
const waitUntilExit = () => new Promise<void>(() => {});Or better, create a mechanism to resolve the promise when the dev session should exit.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli-v3/src/deploy/buildImage.ts (1)
642-653:⚠️ Potential issue | 🟡 MinorMissing Docker Hub logout on build failure.
When the build fails (exitCode !== 0), the code logs out from
cloudRegistryHostbut doesn't log out from Docker Hub ifloggedInToDockerHubis true. This is inconsistent with the success path at lines 698-701.🔧 Proposed fix
if (buildProcess.exitCode !== 0) { if (cloudRegistryHost) { logger.debug(`Logging out from docker registry: ${cloudRegistryHost}`); await x("docker", ["logout", cloudRegistryHost]); } + + if (loggedInToDockerHub) { + logger.debug("Logging out from Docker Hub"); + await x("docker", ["logout"]); + } return { ok: false as const, error: "Error building image", logs: extractLogs(errors), }; }
🤖 Fix all issues with AI agents
In @.changeset/fix-docker-hub-rate-limit-2911.md:
- Line 5: The changeset entry contains a duplicated word "checking checking" in
the release note string; edit the changeset text to remove the duplicate so it
reads "checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables
and logging into Docker Hub before building." Make this single-word fix in the
changeset entry where the phrase appears.
In `@packages/cli-v3/src/commands/dev.ts`:
- Around line 182-206: The dev session is exiting immediately because
devInstance.waitUntilExit is a no-op; change waitUntilExit (the method returned
by startDev / defined in devSession.ts) to return a Promise that only resolves
when the session stops (i.e., when devInstance.stop() is called) or when an
external termination signal occurs; implement this by wiring stop() to resolve a
stored Promise (or awaiting an EventEmitter/AbortSignal) so that await
devInstance.waitUntilExit() blocks until cleanup/signalHandler triggers stop(),
ensuring the bundler/watch stays alive until explicit stop.
In `@packages/core/src/v3/consoleInterceptor.ts`:
- Around line 95-113: In ConsoleInterceptor's stdio forwarding (when
this.sendToStdIO and this.originalConsole are set) the INFO branch currently
always calls this.originalConsole.log(...args); change the INFO handling in the
switch (where severityNumber === SeverityNumber.INFO) to inspect the
severityText parameter and call this.originalConsole.info(...args) when
severityText indicates "Info" (otherwise fallback to
this.originalConsole.log(...args)); keep all other severity branches (WARN,
ERROR, DEBUG, default) unchanged so console.info() is routed to
originalConsole.info() while console.log() still uses originalConsole.log().
🧹 Nitpick comments (3)
packages/cli-v3/src/commands/dev.ts (1)
291-291: EmptywaitUntilExitfunction body.This no-op function causes the dev command to proceed directly to the
finallyblock after starting. If this is intentional, consider adding a comment explaining why blocking is unnecessary here.💡 Suggested documentation
- const waitUntilExit = async () => { }; + // Process stays alive via the config watcher; signal handlers manage cleanup + const waitUntilExit = async () => {};packages/cli-v3/src/commands/update.test.ts (2)
10-51: Consider the guideline on avoiding mocks.The coding guidelines state "Use vitest exclusively for testing and never mock anything - use testcontainers instead." However, testcontainers are primarily for database/Redis testing, not CLI operations.
For CLI unit tests, mocking package manager operations, file system, and user prompts is a pragmatic approach since these operations can't be containerized. If the team prefers integration tests, you could create actual package.json files in a temp directory and run real install commands, but that would significantly slow down tests and add complexity.
The current mocking approach is reasonable for this use case. If the guidelines should exclude CLI tests from the "no mocks" rule, consider updating them for clarity.
74-112: Tests cover the primary scenarios well.The four test cases verify:
- npm with
--no-engine-strict- pnpm with
--config.engine-strict=false- yarn with
--ignore-engines- npm without engine flags when
ignoreEngines: falseConsider adding coverage for edge cases:
- When
packageManagerdetection returnsundefined(should not add any flags)- When an unsupported package manager like
bunis detected (should not add engine flags)💡 Suggested additional test cases
it("should NOT pass engine flags when packageManager is undefined", async () => { vi.mocked(nypm.detectPackageManager).mockResolvedValue(undefined as any); await updateTriggerPackages(".", { ignoreEngines: true } as any, true, true); expect(nypm.installDependencies).toHaveBeenCalledWith(expect.objectContaining({ args: [], })); }); it("should NOT pass engine flags for unsupported package managers (bun)", async () => { vi.mocked(nypm.detectPackageManager).mockResolvedValue({ name: "bun", command: "bun", version: "1.0.0" } as any); await updateTriggerPackages(".", { ignoreEngines: true } as any, true, true); expect(nypm.installDependencies).toHaveBeenCalledWith(expect.objectContaining({ args: [], })); });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/fix-console-interceptor-2900.md.changeset/fix-docker-hub-rate-limit-2911.md.changeset/fix-github-install-node-version-2913.md.changeset/fix-orphaned-workers-2909.mdpackages/cli-v3/src/commands/deploy.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/core/src/v3/consoleInterceptor.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
packages/cli-v3/src/commands/update.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
packages/cli-v3/src/commands/update.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/cli-v3/src/commands/update.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
packages/cli-v3/src/commands/update.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
{packages,integrations}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Add a changeset when modifying any public package in
packages/*orintegrations/*usingpnpm run changeset:add
Files:
packages/cli-v3/src/commands/update.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
packages/core/src/v3/consoleInterceptor.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
packages/cli-v3/src/commands/update.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
packages/cli-v3/src/commands/update.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
packages/cli-v3/src/commands/update.test.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Default to patch version for changesets for bug fixes and minor changes; confirm with maintainers before selecting minor or major
Applied to files:
.changeset/fix-github-install-node-version-2913.md
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to {packages,integrations}/**/* : Add a changeset when modifying any public package in `packages/*` or `integrations/*` using `pnpm run changeset:add`
Applied to files:
.changeset/fix-github-install-node-version-2913.mdpackages/cli-v3/src/commands/update.test.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Applied to files:
.changeset/fix-github-install-node-version-2913.mdpackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-14T19:24:39.536Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2685
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx:1234-1257
Timestamp: 2025-11-14T19:24:39.536Z
Learning: In the trigger.dev project, version validation for the `useNativeBuildServer` setting cannot be performed at the settings form level because the SDK version is only known at build/deployment time, not when saving project settings.
Applied to files:
.changeset/fix-github-install-node-version-2913.md
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `trigger.dev/sdk/v3` for all imports in Trigger.dev tasks
Applied to files:
.changeset/fix-github-install-node-version-2913.md.changeset/fix-orphaned-workers-2909.mdpackages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-10-08T11:48:12.327Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
Applied to files:
.changeset/fix-orphaned-workers-2909.md
📚 Learning: 2025-11-27T16:26:44.496Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/executing-commands.mdc:0-0
Timestamp: 2025-11-27T16:26:44.496Z
Learning: For running tests, navigate into the package directory and run `pnpm run test --run` to enable single-file test execution (e.g., `pnpm run test ./src/engine/tests/ttl.test.ts --run`)
Applied to files:
packages/cli-v3/src/commands/update.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
packages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/deploy.ts
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest exclusively for testing and never mock anything - use testcontainers instead
Applied to files:
packages/cli-v3/src/commands/update.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to packages/trigger-sdk/**/*.{ts,tsx} : In the Trigger.dev SDK (packages/trigger-sdk), prefer isomorphic code like fetch and ReadableStream instead of Node.js-specific code
Applied to files:
packages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Run `npx trigger.devlatest dev` to start the Trigger.dev development server
Applied to files:
packages/cli-v3/src/commands/dev.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export tasks with unique IDs within the project to enable proper task discovery and execution
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure build process in trigger.config.ts using `build` object with external packages, extensions, and JSX settings
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to **/*.{ts,tsx} : Always import tasks from `trigger.dev/sdk`, never use `trigger.dev/sdk/v3` or deprecated `client.defineJob` pattern
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `task()` function from `trigger.dev/sdk/v3` to define tasks with id and run properties
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure Trigger.dev project in `trigger.config.ts` using `defineConfig()` with project ref and task directories
Applied to files:
packages/cli-v3/src/commands/deploy.ts
🧬 Code graph analysis (1)
packages/cli-v3/src/commands/deploy.ts (2)
packages/cli-v3/src/commands/update.ts (1)
updateTriggerPackages(51-304)packages/cli-v3/src/utilities/cliOutput.ts (5)
isLinksSupported(7-7)chalkError(24-26)cliLink(140-145)chalkWarning(28-30)chalkGrey(20-22)
🔇 Additional comments (14)
.changeset/fix-console-interceptor-2900.md (1)
1-5: LGTM!Changeset is properly formatted with appropriate patch-level versioning for this bug fix.
.changeset/fix-github-install-node-version-2913.md (1)
1-5: LGTM!Changeset is properly formatted with appropriate patch-level versioning for this deployment fix.
packages/cli-v3/src/commands/update.ts (3)
18-22: LGTM!The
ignoreEnginesoption is properly added to the UpdateCommandOptions schema by picking it from CommonCommandOptions.
261-277: LGTM!The engine-ignoring flags are correctly mapped for each package manager:
- npm:
--no-engine-strict- pnpm:
--config.engine-strict=false- yarn:
--ignore-enginesThe guard condition properly checks both
ignoreEnginesandpackageManagerbefore applying flags.
278-282: LGTM!Error message formatting is unchanged in substance.
packages/cli-v3/src/commands/dev.ts (1)
173-176: LGTM!Minor reformatting of error message template string.
.changeset/fix-orphaned-workers-2909.md (1)
1-5: LGTM!Changeset is properly formatted with appropriate patch-level versioning. The description accurately reflects the signal handling fix for orphaned workers.
packages/cli-v3/src/deploy/buildImage.ts (3)
476-508: Good security practice using--password-stdin.The Docker Hub authentication correctly uses stdin to pass the password, avoiding exposure in process listings. The login state tracking with
loggedInToDockerHubis clean.
585-592: LGTM!Cache argument reformatting maintains equivalent functionality.
698-701: LGTM!Proper cleanup of Docker Hub session on successful build completion.
packages/cli-v3/src/commands/deploy.ts (3)
255-255: Nice: passignoreEnginesinto the update step.This keeps the update flow consistent with the new package-manager flags.
492-493: Output/link template consolidation looks good.The inline templates preserve the existing env selection while making the output paths more compact.
Also applies to: 710-711, 734-735, 742-743, 786-787, 1086-1087, 1153-1154, 1236-1237, 1251-1252, 1277-1283, 1295-1296, 1302-1303, 1314-1315, 1321-1322, 1333-1334, 1340-1341, 1359-1365
1200-1203: Log-level styling preserved.Warn/debug formatting remains intact after the ternary refactor.
packages/core/src/v3/consoleInterceptor.ts (1)
16-63: Good: capture and restore the original console methods.This keeps interceptors intact and avoids leaving the console in a patched state after completion.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "@trigger.dev/cli-v3": patch | ||
| --- | ||
|
|
||
| Fix: Native build server failed with Docker Hub rate limits. Added support for checking checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (#2911) |
There was a problem hiding this comment.
Fix duplicated word in the changeset entry.
Minor typo (“checking checking”) in user-facing release notes.
📝 Suggested fix
-Fix: Native build server failed with Docker Hub rate limits. Added support for checking checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (`#2911`)
+Fix: Native build server failed with Docker Hub rate limits. Added support for checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (`#2911`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fix: Native build server failed with Docker Hub rate limits. Added support for checking checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (#2911) | |
| Fix: Native build server failed with Docker Hub rate limits. Added support for checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (`#2911`) |
🤖 Prompt for AI Agents
In @.changeset/fix-docker-hub-rate-limit-2911.md at line 5, The changeset entry
contains a duplicated word "checking checking" in the release note string; edit
the changeset text to remove the duplicate so it reads "checking
`DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging
into Docker Hub before building." Make this single-word fix in the changeset
entry where the phrase appears.
| let devInstance: Awaited<ReturnType<typeof startDev>> | undefined; | ||
|
|
||
| const cleanup = async () => { | ||
| if (devInstance) { | ||
| await devInstance.stop(); | ||
| } | ||
| }; | ||
|
|
||
| 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(); | ||
| } finally { | ||
| await watcher?.stop(); | ||
| process.off("SIGINT", signalHandler); | ||
| process.off("SIGTERM", signalHandler); | ||
| await cleanup(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the dev.ts file around the lines in question
cat -n packages/cli-v3/src/commands/dev.ts | sed -n '280,310p'Repository: triggerdotdev/trigger.dev
Length of output: 857
🏁 Script executed:
# Let's also see the full context of the try-finally block and startDev
cat -n packages/cli-v3/src/commands/dev.ts | sed -n '170,210p'Repository: triggerdotdev/trigger.dev
Length of output: 1442
🏁 Script executed:
# Search for startDev function definition to understand the full lifecycle
rg -n 'const startDev|function startDev' packages/cli-v3/src/commands/dev.ts -A 30Repository: triggerdotdev/trigger.dev
Length of output: 1088
🏁 Script executed:
# Check if there's a watcher that might keep the process alive
rg -n 'watcher\|event loop\|setInterval\|setTimeout' packages/cli-v3/src/commands/dev.ts | head -20Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
# Search for watchConfig implementation
rg -n 'export.*watchConfig|const watchConfig|function watchConfig' packages/cli-v3/src/ -A 5 --type=tsRepository: triggerdotdev/trigger.dev
Length of output: 392
🏁 Script executed:
# Check what bootDevSession does
rg -n 'bootDevSession' packages/cli-v3/src/commands/dev.ts -B 2 -A 15Repository: triggerdotdev/trigger.dev
Length of output: 1399
🏁 Script executed:
# See if there are any imports or setup that might explain the lifecycle
head -50 packages/cli-v3/src/commands/dev.tsRepository: triggerdotdev/trigger.dev
Length of output: 2316
🏁 Script executed:
# Look for any existing test files or documentation that explains the expected behavior
fd -e test.ts -e spec.ts packages/cli-v3/src/commands/dev | head -5Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
# Let's examine the watchConfig implementation in config.ts to see if it sets up file watchers that keep the process alive
cat -n packages/cli-v3/src/config.ts | sed -n '62,150p'Repository: triggerdotdev/trigger.dev
Length of output: 3206
🏁 Script executed:
# Check the return type of watchConfig to understand what gets returned
rg -n 'return {' packages/cli-v3/src/config.ts | head -10Repository: triggerdotdev/trigger.dev
Length of output: 148
🏁 Script executed:
# Let's see what the watcher setup looks like - search for chokidar or file watch setup
rg -n 'chokidar\|watch\|FSWatcher' packages/cli-v3/src/config.ts -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 51
🏁 Script executed:
# Check devSession to understand what startDevSession returns
rg -n 'export.*startDevSession|function startDevSession' packages/cli-v3/src/dev/devSession.ts -A 20 | head -40Repository: triggerdotdev/trigger.dev
Length of output: 698
🏁 Script executed:
# Get the full startDevSession function to see what DevSessionInstance contains and what it does
cat -n packages/cli-v3/src/dev/devSession.ts | sed -n '50,150p'Repository: triggerdotdev/trigger.dev
Length of output: 3562
🏁 Script executed:
# Check the DevSessionInstance type definition
rg -n 'type DevSessionInstance|interface DevSessionInstance' packages/cli-v3/src/dev/devSession.ts -A 10Repository: triggerdotdev/trigger.dev
Length of output: 274
🏁 Script executed:
# Let's check what startWorkerRuntime returns and if it keeps the process alive
rg -n 'startWorkerRuntime|return {' packages/cli-v3/src/dev/devSession.ts | head -20Repository: triggerdotdev/trigger.dev
Length of output: 186
🏁 Script executed:
# Look at the end of startDevSession to see what it returns
cat -n packages/cli-v3/src/dev/devSession.ts | tail -100Repository: triggerdotdev/trigger.dev
Length of output: 3334
The waitUntilExit no-op creates a critical bug: the dev session terminates immediately after starting.
The bundler is started in watch mode (line 200 of devSession.ts, via bundleWorker with watch: true), which keeps the process alive by monitoring file changes. However:
await devInstance.waitUntilExit()(line 291 ofdev.ts) is an empty async function that returns immediately- The
finallyblock (lines 202-206) then executes and callscleanup() cleanup()callsdevInstance.stop(), which stops the bundler and file watchers- The dev session terminates before any user interaction
In normal operation (without signals), the dev session would start and immediately stop, preventing the CLI from functioning. The signal handler flow (SIGINT/SIGTERM → cleanup() → process.exit(0)) would work correctly if a user manually terminates, but the command would not stay alive waiting for that signal.
The waitUntilExit function needs to block indefinitely while the dev session is running, not be a no-op.
🤖 Prompt for AI Agents
In `@packages/cli-v3/src/commands/dev.ts` around lines 182 - 206, The dev session
is exiting immediately because devInstance.waitUntilExit is a no-op; change
waitUntilExit (the method returned by startDev / defined in devSession.ts) to
return a Promise that only resolves when the session stops (i.e., when
devInstance.stop() is called) or when an external termination signal occurs;
implement this by wiring stop() to resolve a stored Promise (or awaiting an
EventEmitter/AbortSignal) so that await devInstance.waitUntilExit() blocks until
cleanup/signalHandler triggers stop(), ensuring the bundler/watch stays alive
until explicit stop.
| 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; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the consoleInterceptor.ts file to understand the full context
cat -n packages/core/src/v3/consoleInterceptor.ts | head -150Repository: triggerdotdev/trigger.dev
Length of output: 5440
🏁 Script executed:
#!/bin/bash
# Search for console.info wrapping or assignment patterns
rg -n 'console\.info\s*=' --type ts --type tsx
rg -n 'console\.info\s*:' --type ts --type tsx
# Check for any console.info patches/spies
rg -n '(jest\.spyOn|sinon|spy).*console\.info' --type ts --type tsx
# Search for ConsoleInterceptor usage
rg -n 'ConsoleInterceptor' --type ts --type tsxRepository: triggerdotdev/trigger.dev
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for console.info wrapping or assignment patterns
rg -n 'console\.info\s*=' -g '*.ts' -g '*.tsx'
# Check for console.info patches/spies
rg -n '(jest\.spyOn|sinon|spy).*console\.info' -g '*.ts' -g '*.tsx'
# Search for ConsoleInterceptor usage and tests
rg -n 'ConsoleInterceptor' -g '*.ts' -g '*.tsx' -g '*.js' | head -30Repository: triggerdotdev/trigger.dev
Length of output: 2086
🏁 Script executed:
#!/bin/bash
# Look at the test file to understand expected behavior
cat -n packages/core/test/taskExecutor.test.ts | sed -n '1900,1950p'
# Also check if there are any specific tests for console.info routing
rg -n 'console\.info' -g '*.test.ts' -B2 -A2 packages/core/test/Repository: triggerdotdev/trigger.dev
Length of output: 1525
🏁 Script executed:
#!/bin/bash
# Search for console.info in test files
rg -n 'console\.info' -g '*.test.ts' -B3 -A3
# Search for console method testing in consoleInterceptor context
rg -n 'console\.(log|info|warn|error|debug)' packages/core/test/ -g '*.test.ts' -B2 -A2 | head -100Repository: triggerdotdev/trigger.dev
Length of output: 7205
🏁 Script executed:
#!/bin/bash
# Check for any console method wrapping or middleware in the codebase
rg -n 'originalConsole' -B3 -A3 packages/core/src/v3/
# Check if info/log distinction matters in the logger.emit calls
rg -n 'severityText' packages/core/src/v3/consoleInterceptor.ts -B2 -A2Repository: triggerdotdev/trigger.dev
Length of output: 4314
Preserve console.info() routing to maintain consistency with logger output.
Both console.log() and console.info() map to SeverityNumber.INFO, but the current code always dispatches INFO severity to originalConsole.log() (line 99), bypassing originalConsole.info(). The severityText parameter already distinguishes these calls ("Log" vs "Info" at lines 72 and 76) and is used in logger.emit(), so the distinction should also be applied when delegating to stdio. Route INFO calls based on severityText so info() calls use originalConsole.info().
🛠️ Suggested fix
switch (severityNumber) {
case SeverityNumber.INFO:
- this.originalConsole.log(...args);
+ (severityText === "Info" ? this.originalConsole.info : this.originalConsole.log)(
+ ...args
+ );
break;🤖 Prompt for AI Agents
In `@packages/core/src/v3/consoleInterceptor.ts` around lines 95 - 113, In
ConsoleInterceptor's stdio forwarding (when this.sendToStdIO and
this.originalConsole are set) the INFO branch currently always calls
this.originalConsole.log(...args); change the INFO handling in the switch (where
severityNumber === SeverityNumber.INFO) to inspect the severityText parameter
and call this.originalConsole.info(...args) when severityText indicates "Info"
(otherwise fallback to this.originalConsole.log(...args)); keep all other
severity branches (WARN, ERROR, DEBUG, default) unchanged so console.info() is
routed to originalConsole.info() while console.log() still uses
originalConsole.log().
- Include reproduction scripts for Sentry (triggerdotdev#2900) and engine strictness (triggerdotdev#2913) - Include PR body drafts for consolidated tracking
| export const UpdateCommandOptions = CommonCommandOptions.pick({ | ||
| logLevel: true, | ||
| skipTelemetry: true, | ||
| ignoreEngines: true, | ||
| }); |
There was a problem hiding this comment.
🔴 ignoreEngines option never works because it's missing from CommonCommandOptions
The ignoreEngines feature will never work because the property is not defined in CommonCommandOptions but UpdateCommandOptions tries to pick it.
Click to expand
Root Cause
In packages/cli-v3/src/commands/update.ts:18-21, the code uses CommonCommandOptions.pick() to include ignoreEngines:
export const UpdateCommandOptions = CommonCommandOptions.pick({
logLevel: true,
skipTelemetry: true,
ignoreEngines: true, // This key doesn't exist!
});However, CommonCommandOptions in packages/cli-v3/src/cli/common.ts:12-17 only defines:
export const CommonCommandOptions = z.object({
apiUrl: z.string().optional(),
logLevel: z.enum([...]).default("log"),
skipTelemetry: z.boolean().default(false),
profile: z.string().default(...),
});Zod's .pick() silently ignores keys that don't exist in the source schema. This means ignoreEngines will never be part of UpdateCommandOptions, and options.ignoreEngines will always be undefined.
Impact
The ignoreEngines flag passed in deploy.ts:255 ({ ...options, ignoreEngines: true }) will have no effect because the condition at update.ts:263 (if (options.ignoreEngines && packageManager)) will always be false since options.ignoreEngines is undefined.
This defeats the purpose of fixing #2913 - engine checks during deployment install phase will NOT be ignored.
Recommendation: Add ignoreEngines: z.boolean().default(false) to CommonCommandOptions in packages/cli-v3/src/cli/common.ts, or define UpdateCommandOptions differently by extending with the new property instead of picking a non-existent one.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #2909. Updates \packages/cli-v3/src/commands/dev.ts\ to explicitly handle SIGINT and SIGTERM signals, ensuring that \devInstance.stop()\ is called to terminate child worker processes and remove lockfiles before exiting.