feat(api): defer remote build creation for pending deployments #2536
Conversation
Depot builds have short-lived tokens and their TTL is not exposed in the SDK. As queued deployments can stay in the queue for an arbitrary amount of time, deferring the remote build creation helps avoid expired Depot token issues.
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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. Comment |
![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 (6)
apps/webapp/app/v3/remoteImageBuilder.server.ts (3)
10-12: Prefer explicitreturn undefined.The early exit currently uses bare
return;. Make the return value explicit to match the signature.- return; + return undefined;
64-66: Return a boolean from remoteBuildsEnabled.Right now it returns the last truthy env string, not a boolean. Make this a boolean to avoid accidental truthy/falsy surprises and for clearer intent.
export function remoteBuildsEnabled() { return Boolean(env.DEPOT_TOKEN && env.DEPOT_ORG_ID && env.DEPOT_REGION); }
32-35: Potential duplicate Depot projects under concurrency.Two concurrent calls can both observe a missing
builderProjectId, each create a Depot project, and then race to write it. Consider adding idempotency/a concurrency guard:
- Use a unique name in Depot and first attempt a “get by name” before creating (if supported).
- Or, after creation, write with
updateManyconditioned onbuilderProjectId: nulland, ifcount===0, discard the new id and reload the project (another worker won the race).If helpful, I can draft a small wrapper that attempts “get or create by name” with safe fallbacks.
Also applies to: 37-62
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (1)
64-66: Log the underlying error cause for remote build failures.We currently return 500 without logging; add a structured log to aid triage.
- case "failed_to_create_remote_build": - return json({ error: "Failed to create remote build" }, { status: 500 }); + case "failed_to_create_remote_build": + logger.error("Failed to create remote build", { + deploymentId, + cause: error.cause?.message ?? String(error.cause), + }); + return json({ error: "Failed to create remote build" }, { status: 500 });Optionally consider 502 (Bad Gateway) or 424 (Failed Dependency) if you want to reflect an upstream failure more precisely.
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
90-95: Catch and classify Depot errors during initialization.When
initialStatus !== "PENDING", a Depot outage throws and bubbles as a generic 500. Either proceed without remote build (logging), or raise aServiceValidationErrorfor consistent handling.- // For the `PENDING` initial status, defer the creation of the Depot build until the deployment is started. - // This helps avoid Depot token expiration issues. - const externalBuildData = - payload.initialStatus === "PENDING" - ? undefined - : await createRemoteImageBuild(environment.project); + // For the `PENDING` initial status, defer Depot build creation until start to avoid token TTL issues. + let externalBuildData = + payload.initialStatus === "PENDING" ? undefined : undefined; + if (payload.initialStatus !== "PENDING") { + const [buildErr, build] = await tryCatch( + createRemoteImageBuild(environment.project) + ); + if (buildErr) { + logger.error("Failed to create remote build during init", { + environmentId: environment.id, + projectId: environment.projectId, + cause: buildErr.message, + }); + // Option A: proceed without external build data + externalBuildData = undefined; + // Option B (stricter): throw a classified error + // throw new ServiceValidationError("Failed to create remote build"); + } else { + externalBuildData = build; + } + }If you pick Option B, ensure the caller maps this to a clear API error like in the start route.
Add
ExternalBuildDatato imports if you choose to annotate the variable:import { type InitializeDeploymentRequestBody, type ExternalBuildData } from "@trigger.dev/core/v3";apps/webapp/app/v3/services/deployment.server.ts (1)
50-57: Avoid creating remote builds before winning the PENDING→BUILDING race.If the
updateManybelow loses the race (count=0), we’ve already created a remote Depot build that won’t be used. Consider:
- First flipping the status (PENDING→BUILDING), then creating the remote build, and finally patching
externalBuildData(second update).- Or canceling/ignoring the remote build when
updateManyreturns 0 (if Depot supports cancel).I can sketch a two-step neverthrow flow that minimizes leaks if you want to try it in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts(1 hunks)apps/webapp/app/v3/remoteImageBuilder.server.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(3 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/remoteImageBuilder.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.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/remoteImageBuilder.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/v3/remoteImageBuilder.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/v3/remoteImageBuilder.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/v3/remoteImageBuilder.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
🧬 Code graph analysis (4)
apps/webapp/app/v3/remoteImageBuilder.server.ts (1)
packages/core/src/v3/schemas/api.ts (2)
ExternalBuildData(388-392)ExternalBuildData(394-394)
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
apps/webapp/app/v3/remoteImageBuilder.server.ts (1)
createRemoteImageBuild(7-30)
apps/webapp/app/v3/services/deployment.server.ts (3)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)apps/webapp/app/v3/remoteImageBuilder.server.ts (1)
createRemoteImageBuild(7-30)packages/core/src/v3/schemas/api.ts (2)
ExternalBuildData(388-392)ExternalBuildData(394-394)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (1)
packages/core/src/v3/apps/http.ts (1)
json(65-75)
⏰ 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: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 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 (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/v3/remoteImageBuilder.server.ts (2)
2-3: Type‑only imports: good change.Keeps runtime bundle clean and aligns with our TS guidelines.
7-9: Explicit return type on createRemoteImageBuild is correct.Makes the undefined path explicit for callers using neverthrow.
apps/webapp/app/v3/services/deployment.server.ts (2)
60-73: Update payload shape looks correct.Merges incoming updates, persists
externalBuildData, flips status, and setsstartedAt; guarded bystatus: "PENDING". Good concurrency hygiene.
85-97: Timeout extension mapping LGTM.Error type classification is consistent with the route’s “ignore for now” behavior.
Depot builds have short-lived tokens and their TTL is not exposed in the SDK. As queued deployments can stay in the queue for an arbitrary amount of time, deferring the remote build creation helps avoid expired Depot token issues.