Fixes for internal error reattempts #1436
Conversation
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing error handling and control flow in task management and monitoring services. Key changes include the introduction of new error codes, improvements in the handling of task run statuses, and updates to message schemas to support additional features. The Changes
Possibly related PRs
Suggested reviewers
Tip You can disable sequence diagrams in the walkthrough.Disable the 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: 13
🧹 Outside diff range and nitpick comments (6)
apps/webapp/app/components/runs/v3/SpanEvents.tsx (1)
83-100: LGTM: Well-implemented error handling UI with clear user feedback paths.The conditional rendering provides appropriate UI components based on the error context. The implementation aligns well with the PR's objective of improving error handling.
Minor suggestion: Consider consolidating the Button styling props into a preset variant to improve reusability.
+// In your button component or styles file +const BUTTON_VARIANTS = { + contactForm: { + variant: "tertiary/medium", + leadingIconClassName: "text-blue-400", + fullWidth: true, + textAlignLeft: true, + }, +}; -<Button - variant="tertiary/medium" - LeadingIcon={EnvelopeIcon} - leadingIconClassName="text-blue-400" - fullWidth - textAlignLeft -> +<Button + {...BUTTON_VARIANTS.contactForm} + LeadingIcon={EnvelopeIcon} +>apps/webapp/app/v3/services/crashTaskRun.server.ts (1)
32-35: Enhance the deprecation warning message.While adding a deprecation warning is good practice, consider:
- Including guidance on the recommended alternative
- Adding a deprecation timeline
- Considering whether an immediate return is the best approach, as it might lead to silent failures
Consider updating the warning message:
- logger.error("CrashTaskRunService.call: overrideCompletion is deprecated", { runId }); + logger.error( + "CrashTaskRunService.call: overrideCompletion option is deprecated and will be removed in v4. " + + "Please use the new error handling mechanism instead.", + { runId } + );apps/kubernetes-provider/src/taskMonitor.ts (1)
Line range hint
163-203: Consider adding error metrics and structured logging.While the error handling is comprehensive, adding metrics and structured logging would help with monitoring and debugging:
- Track frequency of different error types (OOMKilled, Evicted, etc.)
- Monitor override patterns
- Log memory/disk usage trends
This would help identify systemic issues and optimize resource allocation.
Would you like me to provide an example implementation using a metrics library?
packages/core/src/v3/errors.ts (1)
169-169: Add user-friendly message for TASK_RUN_HEARTBEAT_TIMEOUTWhile making heartbeat timeout non-retryable is correct, we should add an entry in
prettyInternalErrorsto provide clear feedback to users about what happened and how to resolve it.Add this entry to the
prettyInternalErrorsobject:const prettyInternalErrors: Partial<Record<TaskRunInternalError["code"], { message: string; link?: ErrorLink; }>> = { + TASK_RUN_HEARTBEAT_TIMEOUT: { + message: "The task failed to send heartbeat signals within the expected timeframe. This could indicate that the task was blocked or running too slowly.", + link: { + name: "Task Monitoring", + href: links.docs.tasks.monitoring + } + }, // ... existing entries };apps/webapp/app/v3/services/completeAttempt.server.ts (2)
39-44: Ensure Proper Documentation for New OptionsThe newly introduced
CompleteAttemptServiceOptionstype includes optional parameterssupportsRetryCheckpoints,isSystemFailure, andisCrash. Please ensure that these options are well-documented, including their default values and the effects they have on the service's behavior.
Line range hint
561-572: Enhance Error Handling for Checkpoint Creation FailureWhen checkpoint creation fails:
...Consider providing more detailed error information or implementing a retry mechanism for checkpoint creation failures. This can aid in debugging and potentially recover from transient issues.
Would you like assistance in enhancing the error handling logic for checkpoint creation failures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- apps/kubernetes-provider/src/taskMonitor.ts (3 hunks)
- apps/webapp/app/components/runs/v3/SpanEvents.tsx (2 hunks)
- apps/webapp/app/models/taskRun.server.ts (3 hunks)
- apps/webapp/app/v3/failedTaskRun.server.ts (2 hunks)
- apps/webapp/app/v3/handleSocketIo.server.ts (3 hunks)
- apps/webapp/app/v3/requeueTaskRun.server.ts (2 hunks)
- apps/webapp/app/v3/services/completeAttempt.server.ts (13 hunks)
- apps/webapp/app/v3/services/crashTaskRun.server.ts (6 hunks)
- apps/webapp/app/v3/services/updateFatalRunError.server.ts (1 hunks)
- packages/cli-v3/src/entryPoints/deploy-run-controller.ts (1 hunks)
- packages/core/src/v3/apps/process.ts (1 hunks)
- packages/core/src/v3/errors.ts (2 hunks)
- packages/core/src/v3/runtime/noopRuntimeManager.ts (2 hunks)
- packages/core/src/v3/schemas/messages.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/v3/apps/process.ts
🔇 Additional comments (22)
packages/core/src/v3/runtime/noopRuntimeManager.ts (2)
4-4: LGTM!Clean addition of
TaskRunErrorCodesimport that aligns with the PR's objective of standardizing error handling across the codebase.
26-29: Verify error type consistency with error codeThe error type
"INTERNAL_ERROR"seems inconsistent with the error codeCONFIGURED_INCORRECTLY. Consider using a more appropriate error type that better reflects the configuration-related nature of the error, or verify if this combination is intentional.Let's check how these error types and codes are used together elsewhere:
✅ Verification successful
Let me gather more context about the error types used with this error code:
Let me search specifically for error type definitions to understand the error type system better:
Based on the code analysis, I can now generate the final response:
Error type and code combination is correct
The error type
"INTERNAL_ERROR"with error codeCONFIGURED_INCORRECTLYis a valid and intentional combination in the codebase. This is evident from:
- The error code
CONFIGURED_INCORRECTLYis defined in theTaskRunErrorCodesenum and is consistently used with"INTERNAL_ERROR"type across multiple files.- The error handling logic in
errors.tsspecifically handles this combination in theshouldRetryErrorfunction, whereCONFIGURED_INCORRECTLYis treated as a non-retryable internal error.- This pattern is used consistently in other files like
deploy-run-worker.tsandtaskRun.server.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of CONFIGURED_INCORRECTLY to verify the error type pattern rg -A 2 "CONFIGURED_INCORRECTLY" # Search for similar error objects to understand the pattern ast-grep --pattern 'error: { type: $_, code: TaskRunErrorCodes.$_ }'Length of output: 1260
Script:
#!/bin/bash # Search for error type patterns with CONFIGURED_INCORRECTLY rg -B 2 -A 2 'CONFIGURED_INCORRECTLY' --type ts # Search for error type declarations/interfaces ast-grep --pattern 'type: $_'Length of output: 16089
Script:
#!/bin/bash # Search for error type definitions and interfaces rg -B 3 -A 3 'type.*Error' --type ts # Look for the specific error handling in the errors.ts file cat packages/core/src/v3/errors.tsLength of output: 47715
apps/webapp/app/components/runs/v3/SpanEvents.tsx (1)
1-1: LGTM: Import statements are well-organized and support the new error handling UI.The new imports are properly organized and necessary for the enhanced error handling UI components.
Also applies to: 9-10
apps/webapp/app/v3/requeueTaskRun.server.ts (2)
10-10: LGTM! Good practice using centralized error codes.The addition of
TaskRunErrorCodesimport aligns with the PR's objective to improve error handling consistency.
63-63: Verify consistent usage of TASK_RUN_HEARTBEAT_TIMEOUT across the codebase.The change from string literal to
TaskRunErrorCodes.TASK_RUN_HEARTBEAT_TIMEOUTis good. Let's ensure this constant is used consistently for all heartbeat timeout scenarios.✅ Verification successful
Let me gather more information about the error code definition and its usage pattern.
Based on the gathered information, I can now generate the final response:
TASK_RUN_HEARTBEAT_TIMEOUT usage is consistent across the codebase
The constant is properly defined in the schema and used consistently in error handling. The only instances found are:
- Definition in
packages/core/src/v3/schemas/common.ts- Error handling case in
packages/core/src/v3/errors.ts- Usage in
apps/webapp/app/v3/requeueTaskRun.server.tsNo hardcoded string variants of this error code were found, and it's the only timeout-related task run error code in use, indicating proper standardization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining hardcoded heartbeat timeout strings and verify consistent usage of the constant # Test 1: Look for any remaining hardcoded strings echo "Checking for hardcoded timeout strings..." rg -i "heartbeat.*timeout" --type ts # Test 2: Check usage of the constant echo "Checking usage of TaskRunErrorCodes.TASK_RUN_HEARTBEAT_TIMEOUT..." rg "TaskRunErrorCodes\.TASK_RUN_HEARTBEAT_TIMEOUT" --type tsLength of output: 1852
Script:
#!/bin/bash # Check the definition and usage context of TaskRunErrorCodes # Look for the enum/type definition echo "Checking TaskRunErrorCodes definition..." ast-grep --pattern 'export $_ TaskRunErrorCodes = $$$' # Look for error handling patterns around heartbeat timeout echo "Checking error handling patterns..." rg "TASK_RUN_HEARTBEAT_TIMEOUT" -B 2 -A 2 --type ts # Look for similar error codes in the enum echo "Checking similar error codes..." rg "TASK_RUN_.*_TIMEOUT" --type tsLength of output: 5516
apps/webapp/app/models/taskRun.server.ts (3)
6-6: LGTM! Good practice to import error code constants.The addition of
TaskRunErrorCodesimport supports the standardization of error handling across the application.
65-65: LGTM! Appropriate use of TaskRunErrorCodes constant.The replacement of the string literal with
TaskRunErrorCodes.TASK_RUN_CANCELLEDimproves code maintainability and type safety.
97-97:⚠️ Potential issueConsider using a more appropriate error code for parsing failures.
The
CONFIGURED_INCORRECTLYerror code is being used when parsing the error from a failed task run attempt. This might not accurately represent the nature of the error, as it's a parsing failure rather than a configuration issue. Consider using a more semantically appropriate error code likePARSE_ERRORorINVALID_ERROR_FORMATif available inTaskRunErrorCodes.Let's check available error codes:
apps/webapp/app/v3/services/crashTaskRun.server.ts (3)
7-7: LGTM! Good standardization of error codes.The addition of
TaskRunErrorCodesimport helps centralize and standardize error handling across the application.Also applies to: 7-7
67-67: LGTM! Good error code handling.The use of nullish coalescing with TaskRunErrorCodes provides a clean fallback mechanism while maintaining type safety.
Line range hint
1-201: Verify consistent error handling across related services.Since this service is part of a broader error handling system, let's verify that related services use consistent error codes and patterns.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the verification is complete. The results show consistent error handling patterns across related services:
Error handling is consistent across related services
The error handling implementation shows consistent patterns across services:
- All services use the same
TaskRunErrorCodesenum for error codes- Consistent error structure with
type: "INTERNAL_ERROR"and matching fields (code, message, stackTrace)- Proper fallback handling with
??operator for error codes- Related services (UpdateFatalRunError, CompleteAttempt) follow the same patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other services using TaskRunErrorCodes echo "Checking TaskRunErrorCodes usage across services:" rg "TaskRunErrorCodes\." "apps/webapp/app/v3/services" -A 2 echo "\nChecking for potential inconsistent error handling:" rg "type:\s*['\"]INTERNAL_ERROR['\"]" "apps/webapp/app/v3/services" -A 2Length of output: 3649
apps/webapp/app/v3/failedTaskRun.server.ts (2)
115-123: Verify the relationship between isSystemFailure and isCrash flags.The current implementation sets
isSystemFailure: !isCrash, making them mutually exclusive. This means:
- When
isCrash=true, thenisSystemFailure=false- When
isCrash=false, thenisSystemFailure=true- When
isCrash=undefined, thenisSystemFailure=truePlease verify if this is the intended behavior, especially for the undefined case.
Consider adding a comment explaining the relationship between these flags and their impact on the retry behavior.
275-275: Verify the impact of SDK version requirement change.The SDK version requirement has been increased from "3.0.14" to "3.1.0". This change means that more SDK versions will receive the warning "SDK version not recent enough to determine retry config" and potentially have different retry behavior.
Consider:
- Adding a changelog entry for this breaking change
- Documenting the retry behavior differences between SDK versions
- Adding migration guidance for users on SDK versions between 3.0.14 and 3.1.0
Run this script to assess the impact:
✅ Verification successful
Let me analyze the results to verify the impact of the SDK version change:
- The version change affects the retry configuration behavior in
failedTaskRun.server.ts- The code shows that if SDK version < "3.1.0", it logs a warning and won't populate retry.default
- Looking at the codebase:
- RetryConfig is stored in the database schema
- It's used in background worker tasks
- The retry configuration is passed through the API
- The change only affects the default retry behavior, not custom retry configs
Let me verify one more aspect about the retry configuration usage:
Based on the search results, I can verify the impact of the SDK version change:
- The version change from "3.0.14" to "3.1.0" affects the retry configuration behavior in
failedTaskRun.server.ts- The code shows that if SDK version < "3.1.0", it logs a warning "SDK version not recent enough to determine retry config" and won't populate retry.default
- The retry configuration is stored in the database schema and used in background worker tasks
- The change only affects the default retry behavior, not custom retry configs
- The retry configuration is passed through the API and stored in JSON format
Version update is safe and properly documented
The version requirement change from "3.0.14" to "3.1.0" is safe because:
- It only affects the default retry behavior
- The warning message clearly indicates when an SDK version is too old
- Custom retry configurations are unaffected
- The change is properly tracked in the database schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential impacts of the SDK version change # Look for SDK version references and retry configurations # Check for SDK version references echo "Searching for SDK version references:" rg -A 2 "sdkVersion|3\.0\.14|3\.1\.0" # Check for retry configuration usage echo "Searching for retry configuration patterns:" rg "retry\.default|retryConfig"Length of output: 40738
Script:
#!/bin/bash # Search for retry.default usage and retry configuration documentation echo "Searching for retry.default usage:" rg -A 3 "retry\.default" echo "Searching for retry configuration documentation:" fd -t f "README|CHANGELOG" -x cat {}Length of output: 162969
apps/kubernetes-provider/src/taskMonitor.ts (2)
164-165: LGTM! Clear and descriptive variable naming.The new variable name
onlyOverrideExistingErrorbetter describes its purpose compared to the previousoverrideCompletion. The comment also clearly explains that this will only override existing task errors without crashing the run.
203-203: LGTM! Consistent variable usage.The
failureInfoobject correctly uses the renamed variableonlyOverrideExistingError.packages/core/src/v3/errors.ts (1)
182-182: LGTM: TASK_PROCESS_SIGTERM as retryable errorMoving TASK_PROCESS_SIGTERM to the retryable errors section is appropriate. Since SIGTERM can occur due to temporary issues (like system maintenance or resource constraints), allowing retries gives tasks a chance to complete successfully on subsequent attempts. This aligns with the existing error message that indicates uncertainty about the cause.
packages/cli-v3/src/entryPoints/deploy-run-controller.ts (1)
522-527: LGTM! Clean refactoring of exit code logic.The introduction of
isNonZeroExitErrorimproves readability by clearly encapsulating the condition for non-zero exit codes. The logic correctly identifies internal errors with non-zero exit codes using theTaskRunErrorCodesenum.apps/webapp/app/v3/services/updateFatalRunError.server.ts (1)
39-46: Confirm the logic for non-fataltaskRunstatusesThe method returns when the
taskRunis not in a fatal state. Verify that this behavior aligns with the intended workflow. If updates are only applicable to fatal task runs, consider documenting this explicitly for clarity.apps/webapp/app/v3/services/completeAttempt.server.ts (3)
225-242: Verify the Execution Retry Inference LogicThe logic for inferring
executionRetrybased onisCrashorisSystemFailureflags adds complexity:Please verify that this inference correctly handles all scenarios, especially when neither
isCrashnorisSystemFailureis true. Edge cases might lead to unexpected retries or missed retries.
246-259: Ensure Correct Handling ofexecutionRetryInferredPassing
executionRetryInferredto the#retryAttemptmethod:Ensure that the downstream methods utilize
executionRetryInferredappropriately and that it does not introduce unintended side effects in the retry logic.
433-443: Validate Environment Variables in Threshold LogicThe condition that checks the
executionRetry.delayagainstenv.CHECKPOINT_THRESHOLD_IN_MS:...Ensure that
env.CHECKPOINT_THRESHOLD_IN_MSis defined and properly populated in all environments where this code will run. Missing or misconfigured environment variables could lead to unintended behavior.packages/core/src/v3/schemas/messages.ts (1)
255-257: LGTM: Addition of optional fields to 'WORKER_CRASHED' message schema.The inclusion of
overrideCompletionanderrorCodeas optional fields enhances error handling flexibility without affecting existing functionality.
@trigger.dev/react-hooks
@trigger.dev/sdk
@trigger.dev/core
@trigger.dev/build
trigger.dev
commit: |
nicktrn
changed the title
Summary by CodeRabbit
Release Notes
New Features
UpdateFatalRunErrorServiceto manage fatal errors effectively.Bug Fixes
Documentation
Refactor