Various fixes for run engine v1 #1643
Conversation
- Make sure there are connected providers before sending a scheduled attempt message, nack and retry if there are not - Fail runs that fail task heartbeats when pending and locked - More and better logging around shared queue consumer - Fix bug when failing a task run with no attempt
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/webapp/app/v3/failedTaskRun.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces several modifications across multiple files in the Trigger.dev application, focusing on enhancing task run management, error handling, logging, and message sending capabilities. The changes span various services and components, including task run retry logic, queue consumers, socket connections, and logging mechanisms. The modifications aim to improve the robustness and flexibility of the system by refining data retrieval methods, adding conditional message sending, and improving error and message logging. Changes
Sequence DiagramsequenceDiagram
participant MessageSender as ZodMessageSender
participant Socket as SocketNamespace
MessageSender->>Socket: Check socket connections
Socket-->>MessageSender: Return connection status
MessageSender->>MessageSender: Validate can send message
alt Can send message
MessageSender->>Socket: Send message
else Cannot send message
MessageSender-->>Caller: Prevent message sending
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (3)
apps/webapp/app/v3/services/resumeTaskDependency.server.ts (1)
9-9: Consider usingfindUniqueinstead offindFirst.Since you're querying by
idwhich is a unique identifier, usingfindUniquewould be more appropriate as it:
- Leverages the unique constraint for better query performance
- Clearly communicates the intent to find exactly one record
- Prevents potential bugs if the uniqueness constraint is accidentally removed
- const dependency = await this._prisma.taskRunDependency.findFirst({ + const dependency = await this._prisma.taskRunDependency.findUnique({apps/webapp/app/v3/failedTaskRun.server.ts (1)
17-29: Consider improving the constant name and adding documentation.While the implementation is correct, consider:
- Renaming to
TaskRunRetryPayloadSelectto better reflect its purpose- Adding JSDoc comments to document the purpose and structure
+/** + * Defines the fields to select when retrieving a task run for retry processing. + * @property {boolean} id - The task run ID + * @property {Object} attempts - The most recent attempt + * @property {string} lockedById - The ID of the task that locked this run + * @property {string} lockedToVersionId - The ID of the worker version that locked this run + */ -const FailedTaskRunRetryGetPayload = { +const TaskRunRetryPayloadSelect = {packages/core/src/logger.ts (1)
126-162: Add type annotations and defensive coding.The functions work correctly but could benefit from:
- TypeScript type annotations for better type safety
- Defensive coding for edge cases
Consider this implementation:
-function extractStructuredErrorFromArgs(...args: Array<Record<string, unknown> | undefined>) { +function extractStructuredErrorFromArgs(...args: Array<Record<string, unknown> | undefined>): { message: string; stack?: string; name: string; } | undefined { const error = args.find((arg) => arg instanceof Error) as Error | undefined; if (error) { return { message: error.message, - stack: error.stack, + stack: error.stack ?? undefined, name: error.name, }; } - const structuredError = args.find((arg) => arg?.error); + const structuredError = args.find((arg): arg is { error: Error } => + arg?.error instanceof Error + ); if (structuredError && structuredError.error instanceof Error) { return { message: structuredError.error.message, - stack: structuredError.error.stack, + stack: structuredError.error.stack ?? undefined, name: structuredError.error.name, }; } return; } -function extractStructuredMessageFromArgs(...args: Array<Record<string, unknown> | undefined>) { +function extractStructuredMessageFromArgs(...args: Array<Record<string, unknown> | undefined>): string | undefined { // Check to see if there is a `message` key in the args, and if so, return it - const structuredMessage = args.find((arg) => arg?.message); + const structuredMessage = args.find((arg): arg is { message: string } => + typeof arg?.message === 'string' + ); if (structuredMessage) { return structuredMessage.message; } return; }The changes:
- Add explicit return type annotations
- Use type predicates for better type inference
- Add null checks for error stack traces
- Add type checking for message property
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/webapp/app/v3/failedTaskRun.server.ts(2 hunks)apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts(7 hunks)apps/webapp/app/v3/requeueTaskRun.server.ts(2 hunks)apps/webapp/app/v3/services/createTaskRunAttempt.server.ts(1 hunks)apps/webapp/app/v3/services/resumeTaskDependency.server.ts(1 hunks)apps/webapp/app/v3/sharedSocketConnection.ts(1 hunks)packages/core/src/logger.ts(2 hunks)packages/core/src/v3/zodMessageHandler.ts(1 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: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
apps/webapp/app/v3/sharedSocketConnection.ts (1)
83-90: LGTM! Well-implemented socket connection check.The
canSendMessageimplementation correctly verifies socket connectivity by:
- First checking if there are any sockets in the namespace
- Then verifying if at least one socket is connected
apps/webapp/app/v3/requeueTaskRun.server.ts (1)
46-68: LGTM! Robust handling of locked PENDING tasks.The implementation correctly:
- Detects locked PENDING tasks
- Fails them with appropriate error code (TASK_RUN_HEARTBEAT_TIMEOUT)
- Nacks unlocked tasks
- Includes detailed logging for better observability
apps/webapp/app/v3/failedTaskRun.server.ts (1)
31-31: LGTM! Type-safe and optimized query implementation.The changes improve type safety and query optimization by:
- Using a const assertion for better type inference
- Selecting only necessary fields
- Using direct ID references instead of related records
Also applies to: 96-96
packages/core/src/logger.ts (2)
99-100: LGTM! Clean extraction of structured data.The function calls are well-organized and use appropriate variable names for clarity.
105-105: LGTM! Clean conditional inclusion of structured data.Good use of the spread operator and conditional object spread pattern. The
$messageprefix helps distinguish the structured message from the regular message field.Also applies to: 109-109
apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (1)
257-262: LGTM! Improved query flexibility.The changes from
findUniquetofindFirstwith conditional where clause provide better flexibility in searching by eitheridorfriendlyId. The implementation correctly handles the prefix check for determining the search field.packages/core/src/v3/zodMessageHandler.ts (2)
242-242: LGTM! Clean implementation of conditional message sending.Good addition of the optional
canSendMessagefunction with proper typing and initialization.Also applies to: 248-248, 253-253
256-260: LGTM! Simple and effective validation method.The
validateCanSendMessagemethod is well-implemented with:
- Proper handling of undefined callback
- Correct async/await usage
- Clear boolean return type
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (7)
37-37: LGTM! Added required imports.Clean addition of queue-related utility imports.
324-331: LGTM! Enhanced debug logging.Good addition of structured logging with relevant statistics for better observability.
360-363: LGTM! Improved logging conditionals.Smart optimization to skip logging for common "no_message_dequeued" cases while ensuring other results are logged.
384-386: LGTM! Enhanced error handling.Good improvement to propagate errors to the current span for better tracing.
771-771: LGTM! Clearer log message.Updated log message better reflects the action being taken.
892-934: LGTM! Robust attempt scheduling implementation.The implementation correctly:
- Uses span for tracing
- Validates message sending capability
- Handles provider disconnection gracefully
- Includes comprehensive attempt scheduling data
961-962: LGTM! Consistent retry interval.Good addition of next tick interval for consistent retry behavior.
@trigger.dev/build
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/sdk
@trigger.dev/core
commit: |
Summary by CodeRabbit
Logging Enhancements
Error Handling
Message Sending
Database Query Improvements
These changes focus on improving system reliability, logging, and message handling across various components.