Fix deploy timeout issues #1661
Conversation
🦋 Changeset detectedLatest commit: a412b35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
|
Warning Rate limit exceeded@ericallam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a new API endpoint for finalizing deployments with enhanced streaming, logging, and error handling. It modifies the existing deployment services by adding early exit checks and an optional log writer parameter, while the CLI now uses server-sent events and dynamic logging callbacks. Additionally, new types and classes for structured SSE handling have been added, alongside a dependency update in the core package. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as API Endpoint (v3 finalize)
participant S as FinalizeDeploymentV2Service
C->>API: POST /api/v3/deployments/{deploymentId}/finalize
API->>API: Validate request & authenticate
API->>S: Call service (with optional writer)
S-->>API: Return result / stream logs
API->>C: Stream events (ping, log, complete/error)
sequenceDiagram
participant CLI as CLI Client
participant API as API Server (finalize endpoint)
CLI->>API: finalizeDeployment(id, payload, onLog callback)
API->>CLI: Emit SSE events (log, complete, error)
CLI->>CLI: Process events via onLog callback
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 2
🧹 Nitpick comments (5)
apps/webapp/app/routes/api.v3.deployments.$deploymentId.finalize.ts (3)
15-16: Ensure consistent response format for invalid request methods.Here, the response is returned as a plain object rather than using your framework’s standard JSON response helper. Consider using a unified approach across all error responses for better consistency.
- return { status: 405, body: "Method Not Allowed" }; + return json({ error: "Method Not Allowed" }, { status: 405 });
19-23: Improve error clarity for invalid parameters.The current response only returns a generic error message. Include more context or pass along Zod error details for debugging, similar to how your body validation error is handled.
61-63: Evaluate the keep-alive ping interval.A 10-second interval might add overhead, especially for many concurrent connections. Consider a slightly longer interval (e.g., 30 seconds) or an on-demand ping to keep the connection alive without excessive network traffic.
packages/cli-v3/src/apiClient.ts (2)
250-254: Consider more typed or contextualized log callbacks.The
onLog?: (message: string) => voidcallback is flexible but might become unwieldy if logs need additional metadata (levels, timestamps, etc.). Consider an interface or object parameter for structured logs.
259-309: Harden SSE lifecycle handling.While you stop the SSE source after awaiting the promise, consider additional event handling (e.g., an
openevent or a fallback timer) to handle cases where the server never sends a completion event.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/webapp/app/routes/api.v2.deployments.$deploymentId.finalize.ts(1 hunks)apps/webapp/app/routes/api.v3.deployments.$deploymentId.finalize.ts(1 hunks)apps/webapp/app/v3/services/finalizeDeployment.server.ts(1 hunks)apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts(5 hunks)packages/cli-v3/src/apiClient.ts(2 hunks)packages/cli-v3/src/commands/deploy.ts(4 hunks)packages/cli-v3/src/deploy/buildImage.ts(8 hunks)packages/core/package.json(1 hunks)packages/core/src/v3/apiClient/core.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/routes/api.v2.deployments.$deploymentId.finalize.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
apps/webapp/app/routes/api.v3.deployments.$deploymentId.finalize.ts (2)
44-50: Handle abrupt client disconnections to prevent write errors.When the client terminates the connection abruptly, attempting to write to the stream may fail, causing unhandled promise rejections. Consider adding a controller or event listener to handle disconnections gracefully.
93-102: Avoid logging full error messages if they could contain sensitive data.When logging errors, some messages might include internal details. Use caution or masking to ensure sensitive information isn't exposed in logs.
packages/cli-v3/src/apiClient.ts (1)
24-24: Good addition ofzodfetchSSE.This import nicely integrates SSE handling with Zod validation, ensuring all events are typed and validated.
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
46-50: Confirm no re-deployment logic is needed.Returning early when the status is
"DEPLOYED"is fine if you never need to re-run finalization. Otherwise, consider explicitly failing or allowing re-deployment with a specialized parameter or workflow.apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (3)
16-17: LGTM! Thewriterparameter is properly typed.The optional
writerparameter is correctly typed asWritableStreamDefaultWriter, which is appropriate for streaming logs.
87-107: LGTM! Thewriterparameter is correctly passed toexecutePushToRegistry.The function call correctly includes all required parameters and passes the optional
writerparameter.
155-158: LGTM! Thewriterparameter is properly added to the function signature.The optional
writerparameter is correctly added to theexecutePushToRegistryfunction signature.packages/cli-v3/src/commands/deploy.ts (5)
225-226: LGTM! The build spinner messages are more descriptive.The messages have been updated to be more specific about what is being built ("trigger code" instead of "project").
Also applies to: 228-229
331-334: LGTM! The deployment spinner messages are consistent.The messages are consistent between the links-supported and non-links-supported cases.
362-368: LGTM! The build progress is properly logged.The
onLogcallback correctly updates the spinner message with build progress, maintaining consistency between links-supported and non-links-supported cases.
436-440: LGTM! The deployment spinner messages are clear.The messages clearly indicate the deployment phase of the process.
442-455: LGTM! The deployment progress is properly logged.The
onLogcallback correctly updates the spinner message with deployment progress, maintaining consistency between links-supported and non-links-supported cases.packages/cli-v3/src/deploy/buildImage.ts (3)
39-39: LGTM! TheonLogcallback is consistently defined across interfaces.The optional
onLogcallback is properly typed and consistently added to all relevant interfaces.Also applies to: 145-145, 290-290
233-236: LGTM! TheonLogcallback is properly invoked indepotBuildImage.The callback is correctly invoked for each log line, allowing real-time streaming of build logs.
349-349: LGTM! TheonLogcallback is properly invoked inselfHostedBuildImage.The callback is correctly invoked for each log line, ensuring consistent log streaming behavior across build methods.
packages/core/src/v3/apiClient/core.ts (2)
620-622: LGTM! The SSE types are well-defined and flexible.The type definitions provide a robust foundation for type-safe SSE handling:
ZodFetchSSEMessageValueSchemaallows both first-party schemas and discriminated unionsZodFetchSSEMessageCatalogSchemaenables a flexible message catalogZodFetchSSEMessageHandlersensures type-safe message handlersZodFetchSSEOptionsprovides a clean configuration interfaceAlso applies to: 624-626, 628-631, 633-638
686-690: LGTM! ThezodfetchSSEfunction provides a clean API.The function provides a simple way to create SSE connections with type-safe message handling.
packages/core/package.json (1)
200-200: New Dependency for SSE Streaming AddedThe addition of
"eventsource": "^3.0.5"supports enhanced SSE functionality—critical for the new API endpoint managing deployment finalization with real-time log streaming. Please ensure that:
- This version constraint aligns with other SSE-related libraries in the ecosystem (e.g.,
"eventsource-parser": "^3.0.0").- The dependency is tested in environments where SSE streaming is critical to avoid any potential timing or compatibility issues.
| // Now we can write strings directly | ||
| if (writer) { | ||
| for (const line of lines) { | ||
| await writer.write(`event: log\ndata: ${JSON.stringify({ message: line })}\n\n`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for stream writes.
The code writes to the stream without handling potential write errors. Stream writes can fail, especially in case of network issues or if the client disconnects.
Apply this diff to add error handling:
if (writer) {
for (const line of lines) {
- await writer.write(`event: log\ndata: ${JSON.stringify({ message: line })}\n\n`);
+ try {
+ await writer.write(`event: log\ndata: ${JSON.stringify({ message: line })}\n\n`);
+ } catch (error) {
+ logger.error("Failed to write to stream", { error });
+ break;
+ }
}
}📝 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.
| // Now we can write strings directly | |
| if (writer) { | |
| for (const line of lines) { | |
| await writer.write(`event: log\ndata: ${JSON.stringify({ message: line })}\n\n`); | |
| } | |
| } | |
| // Now we can write strings directly | |
| if (writer) { | |
| for (const line of lines) { | |
| try { | |
| await writer.write(`event: log\ndata: ${JSON.stringify({ message: line })}\n\n`); | |
| } catch (error) { | |
| logger.error("Failed to write to stream", { error }); | |
| break; | |
| } | |
| } | |
| } |
| export class ZodFetchSSEResult<TMessageCatalog extends ZodFetchSSEMessageCatalogSchema> { | ||
| private _eventSource: EventSource; | ||
|
|
||
| constructor(private options: ZodFetchSSEOptions<TMessageCatalog>) { | ||
| this._eventSource = new EventSource(options.url, { | ||
| fetch: (input, init) => { | ||
| return fetch(input, { | ||
| ...init, | ||
| ...options.request, | ||
| headers: { | ||
| ...options.request?.headers, | ||
| Accept: "text/event-stream", | ||
| }, | ||
| }); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| public onMessage<T extends keyof TMessageCatalog>( | ||
| type: T, | ||
| handler: ZodFetchSSEMessageHandlers<TMessageCatalog>[T] | ||
| ) { | ||
| this._eventSource.addEventListener(type as string, (event) => { | ||
| const payload = safeJsonParse(event.data); | ||
|
|
||
| if (!payload) { | ||
| return; | ||
| } | ||
|
|
||
| const schema = this.options.messages[type]; | ||
|
|
||
| const result = schema.safeParse(payload); | ||
|
|
||
| if (result.success) { | ||
| handler?.(result.data); | ||
| } else { | ||
| console.error(result.error); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| public stop() { | ||
| this._eventSource.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error event handling.
The ZodFetchSSEResult class handles message events but doesn't handle SSE connection errors. This could lead to silent failures.
Add error event handling:
constructor(private options: ZodFetchSSEOptions<TMessageCatalog>) {
this._eventSource = new EventSource(options.url, {
fetch: (input, init) => {
return fetch(input, {
...init,
...options.request,
headers: {
...options.request?.headers,
Accept: "text/event-stream",
},
});
},
});
+
+ this._eventSource.onerror = (event) => {
+ console.error("SSE connection error:", event);
+ // Optionally, attempt to reconnect or notify the user
+ };
}📝 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.
| export class ZodFetchSSEResult<TMessageCatalog extends ZodFetchSSEMessageCatalogSchema> { | |
| private _eventSource: EventSource; | |
| constructor(private options: ZodFetchSSEOptions<TMessageCatalog>) { | |
| this._eventSource = new EventSource(options.url, { | |
| fetch: (input, init) => { | |
| return fetch(input, { | |
| ...init, | |
| ...options.request, | |
| headers: { | |
| ...options.request?.headers, | |
| Accept: "text/event-stream", | |
| }, | |
| }); | |
| }, | |
| }); | |
| } | |
| public onMessage<T extends keyof TMessageCatalog>( | |
| type: T, | |
| handler: ZodFetchSSEMessageHandlers<TMessageCatalog>[T] | |
| ) { | |
| this._eventSource.addEventListener(type as string, (event) => { | |
| const payload = safeJsonParse(event.data); | |
| if (!payload) { | |
| return; | |
| } | |
| const schema = this.options.messages[type]; | |
| const result = schema.safeParse(payload); | |
| if (result.success) { | |
| handler?.(result.data); | |
| } else { | |
| console.error(result.error); | |
| } | |
| }); | |
| } | |
| public stop() { | |
| this._eventSource.close(); | |
| } | |
| } | |
| export class ZodFetchSSEResult<TMessageCatalog extends ZodFetchSSEMessageCatalogSchema> { | |
| private _eventSource: EventSource; | |
| constructor(private options: ZodFetchSSEOptions<TMessageCatalog>) { | |
| this._eventSource = new EventSource(options.url, { | |
| fetch: (input, init) => { | |
| return fetch(input, { | |
| ...init, | |
| ...options.request, | |
| headers: { | |
| ...options.request?.headers, | |
| Accept: "text/event-stream", | |
| }, | |
| }); | |
| }, | |
| }); | |
| this._eventSource.onerror = (event) => { | |
| console.error("SSE connection error:", event); | |
| // Optionally, attempt to reconnect or notify the user | |
| }; | |
| } | |
| public onMessage<T extends keyof TMessageCatalog>( | |
| type: T, | |
| handler: ZodFetchSSEMessageHandlers<TMessageCatalog>[T] | |
| ) { | |
| this._eventSource.addEventListener(type as string, (event) => { | |
| const payload = safeJsonParse(event.data); | |
| if (!payload) { | |
| return; | |
| } | |
| const schema = this.options.messages[type]; | |
| const result = schema.safeParse(payload); | |
| if (result.success) { | |
| handler?.(result.data); | |
| } else { | |
| console.error(result.error); | |
| } | |
| }); | |
| } | |
| public stop() { | |
| this._eventSource.close(); | |
| } | |
| } |
@trigger.dev/build
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/core
@trigger.dev/sdk
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli-v3/src/apiClient.ts (1)
250-316: LGTM! Consider adding Promise rejection handling.The SSE implementation is well-structured with proper event handling for completion, errors, and logging. The cleanup is handled correctly by stopping the SSE connection.
Consider adding a try-catch block around the Promise to ensure proper error handling:
async finalizeDeployment( id: string, body: FinalizeDeploymentRequestBody, onLog?: (message: string) => void ): Promise<ApiResult<FailDeploymentResponseBody>> { if (!this.accessToken) { throw new Error("finalizeDeployment: No access token"); } + try { let resolvePromise: (value: ApiResult<FailDeploymentResponseBody>) => void; let rejectPromise: (reason: any) => void; const promise = new Promise<ApiResult<FailDeploymentResponseBody>>((resolve, reject) => { resolvePromise = resolve; rejectPromise = reject; }); const source = zodfetchSSE({ url: `${this.apiURL}/api/v3/deployments/${id}/finalize`, request: { method: "POST", headers: { Authorization: `Bearer ${this.accessToken}`, "Content-Type": "application/json", }, body: JSON.stringify(body), }, messages: { error: z.object({ error: z.string() }), log: z.object({ message: z.string() }), complete: FailDeploymentResponseBody, }, }); source.onConnectionError((error) => { rejectPromise({ success: false, error, }); }); source.onMessage("complete", (message) => { resolvePromise({ success: true, data: message, }); }); source.onMessage("error", ({ error }) => { rejectPromise({ success: false, error, }); }); if (onLog) { source.onMessage("log", ({ message }) => { onLog(message); }); } const result = await promise; source.stop(); return result; + } catch (error) { + return { + success: false, + error: error instanceof Error ? error.message : String(error), + }; + } }packages/core/src/v3/apiClient/core.ts (1)
640-694: LGTM! Consider enhancing error logging.The SSE implementation is robust with proper lifecycle management, error handling, and type safety.
Consider enhancing error logging in the
onMessagehandler:public onMessage<T extends keyof TMessageCatalog>( type: T, handler: ZodFetchSSEMessageHandlers<TMessageCatalog>[T] ) { this._eventSource.addEventListener(type as string, (event) => { const payload = safeJsonParse(event.data); if (!payload) { + console.error(`Failed to parse SSE message of type '${type}':`, event.data); return; } const schema = this.options.messages[type]; const result = schema.safeParse(payload); if (result.success) { handler?.(result.data); } else { - console.error(result.error); + console.error(`Invalid SSE message of type '${type}':`, { + error: result.error, + payload, + }); } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli-v3/src/apiClient.ts(2 hunks)packages/core/src/v3/apiClient/core.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/cli-v3/src/apiClient.ts (1)
24-24: LGTM!The import of
zodfetchSSEis correctly added to support the new SSE functionality.packages/core/src/v3/apiClient/core.ts (1)
620-638: LGTM! Well-structured type definitions.The type definitions are well-designed with proper handling of discriminated unions and flexible message catalogs.
Summary by CodeRabbit
New Features
Chores