Fix for v4 pending versions not getting dequeued after a deploy #2411
Conversation
We didn’t call `scheduleEnqueueRunsForBackgroundWorker` in the right place…
|
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webapp/app/v3/services/finalizeDeployment.server.ts (2)
12-13: Avoid importing from root@trigger.dev/corein the webapp; replacetryCatchwith native try/catch.Per coding guidelines for apps/webapp, don’t import from the root
@trigger.dev/core. Since this usage is a simple promise await, prefer native try/catch and drop the import.Apply this diff to remove the root import:
import { engine } from "../runEngine.server"; -import { tryCatch } from "@trigger.dev/core";A corresponding change to the scheduling block is suggested in a separate comment below.
121-131: Use native try/catch for scheduling and add context to error logs.Removes the guideline-violating import, simplifies flow, and enriches observability with identifiers.
- if (deployment.worker.engine === "V2") { - const [schedulePendingVersionsError] = await tryCatch( - engine.scheduleEnqueueRunsForBackgroundWorker(deployment.worker.id) - ); - - if (schedulePendingVersionsError) { - logger.error("Error scheduling pending versions", { - error: schedulePendingVersionsError, - }); - } - } + if (deployment.worker.engine === "V2") { + try { + await engine.scheduleEnqueueRunsForBackgroundWorker(deployment.worker.id); + } catch (error) { + logger.error("Error scheduling pending versions", { + error, + workerId: deployment.worker.id, + deploymentId: deployment.id, + environmentId: authenticatedEnv.id, + }); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/webapp/app/v3/services/finalizeDeployment.server.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/v3/services/finalizeDeployment.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/v3/services/finalizeDeployment.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/v3/services/finalizeDeployment.server.ts
🧬 Code Graph Analysis (1)
apps/webapp/app/v3/services/finalizeDeployment.server.ts (2)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)apps/webapp/app/v3/runEngine.server.ts (1)
engine(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/v3/services/finalizeDeployment.server.ts (2)
121-131: No changes needed fortryCatchusage
ThetryCatchutility is defined as an async function that accepts a Promise and returns[error, result]. Core tests inpackages/core/test/utils.test.tsconfirm its contract and tuple order. InfinalizeDeployment.server.ts, you’re passing a Promise totryCatchand only destructuring the error—this aligns perfectly with its intended use.
121-131: Engine gating is correct for existing versions
Our codebase only defines “V1” and “V2” as valid engine values—no “V4” literal or enum member is present. Theif (deployment.worker.engine === "V2")block correctly targets the only background‐worker engine that supports scheduling pending versions today.
• If and when a “V4” engine is officially introduced, you’ll need to add its literal/enum and then extend this check accordingly.Likely an incorrect or invalid review comment.
We didn’t call
scheduleEnqueueRunsForBackgroundWorkerin the rightplace…