-
Notifications
You must be signed in to change notification settings - Fork 156
🛠️ Fix failing workflow test and implement comprehensive retry handling #2167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Re-enabled the previously skipped test for handling schema update failure and adjusted the recursion limit from 20 to 40 to accommodate workflow execution. - Enhanced error handling in tests to match against multiple error scenarios, ensuring robustness in the chat workflow. - Improved annotations in `langGraphUtils.ts` to include default values and reducers for better state management.
…handling in workflow nodes - Renamed NODE_NAME constants across multiple workflow nodes for consistency, changing from `analyzeRequirementsNode` to `analyzeRequirements`, `designSchemaNode` to `designSchema`, and others similarly. - Improved error handling in `designSchemaNode`, `finalizeArtifactsNode`, `executeDDLNode`, `generateDDLNode`, `generateUsecaseNode`, `prepareDMLNode`, `reviewDeliverablesNode`, and `validateSchemaNode` to include retry logic and clearer logging. - Updated the workflow service to utilize a new function `getNextNodeOrEnd` for determining the next node based on error state and retry count, enhancing the flow control of the chat workflow.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Updates to Preview Branch (fix-test-error) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
userInput: Annotation<string>({ | ||
reducer: (_, newValue: string) => newValue, | ||
default: () => '', | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what happened:
LangGraph's default LastValue
annotation only allows one write per channel per workflow step. However, when our nodes retry due to errors, they spread the entire state (...state), which includes all existing channel values.
This caused LangGraph to think we were writing to channels like userInput multiple times in the same step, triggering "Invalid update for channel"
errors.
Adding custom reducers that explicitly handle duplicate values (by simply returning the new value) tells LangGraph how to resolve these "conflicts" and allows retries to work properly.
about reducer
https://langchain-ai.github.io/langgraph/concepts/low_level/#default-reducer
PR Reviewer Guide 🔍(Review updated until commit 622473d)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 622473d
Previous suggestionsSuggestions up to commit 7e7fd8b
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes a failing workflow test and implements comprehensive retry handling across workflow nodes to improve error management and prevent infinite retry loops.
- Reactivates a skipped test and increases the recursion limit.
- Standardizes node naming and error handling across multiple workflow nodes.
- Updates retry logic and conditional edge transitions in the workflow service.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
workflow.test.ts | Reactivated the failing test, updated recursion limit, and modified error assertions to use regex matching. |
langGraphUtils.ts | Adjusted the default recursion limit and enhanced annotation definitions. |
services/workflow.ts | Refactored retry logic into getNextNodeOrEnd to direct flow based on error state and retry count. |
validateSchemaNode.ts, reviewDeliverablesNode.ts, prepareDMLNode.ts, generateDDLNode.ts, finalizeArtifactsNode.ts, executeDDLNode.ts, designSchemaNode.ts, analyzeRequirementsNode.ts | Standardized NODE_NAME constants and integrated consistent try-catch and retry count handling across all nodes. |
Comments suppressed due to low confidence (2)
frontend/internal-packages/agent/src/chat/workflow/nodes/finalizeArtifactsNode.ts:34
- Review the decision to throw an error on save failures and ensure this behavior aligns with the overall workflow error handling strategy for consistent failure management.
throw new Error(`Failed to save error message: ${saveResult.error}`)
frontend/internal-packages/agent/src/chat/workflow/services/workflow.ts:38
- Verify that the constant 'END' is defined or imported within this module to prevent potential reference errors during workflow execution.
return END
// Increment retry count and set error | ||
return { | ||
...state, | ||
error: errorMessage, | ||
retryCount: { | ||
...state.retryCount, | ||
[NODE_NAME]: retryCount + 1, | ||
}, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the retry count increment logic into a shared helper to reduce repetition and improve maintainability across workflow nodes.
// Increment retry count and set error | |
return { | |
...state, | |
error: errorMessage, | |
retryCount: { | |
...state.retryCount, | |
[NODE_NAME]: retryCount + 1, | |
}, | |
} | |
} | |
} | |
// Increment retry count and set error using helper | |
return incrementRetryCount(state, NODE_NAME, errorMessage) | |
} | |
} | |
/** | |
* Helper function to increment retry count and set error in state. | |
* @param state - The current workflow state. | |
* @param nodeName - The name of the node. | |
* @param errorMessage - The error message to set. | |
* @returns Updated workflow state with incremented retry count and error. | |
*/ | |
function incrementRetryCount( | |
state: WorkflowState, | |
nodeName: string, | |
errorMessage: string, | |
): WorkflowState { | |
const retryCount = state.retryCount[nodeName] ?? 0 | |
return { | |
...state, | |
error: errorMessage, | |
retryCount: { | |
...state.retryCount, | |
[nodeName]: retryCount + 1, | |
}, | |
} | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Introduced a new utility function `incrementRetryCount` to streamline error handling and retry count management in multiple workflow nodes. - Removed redundant retry count logic from `analyzeRequirementsNode`, `designSchemaNode`, `executeDDLNode`, `finalizeArtifactsNode`, `generateDDLNode`, `generateUsecaseNode`, `prepareDMLNode`, `reviewDeliverablesNode`, and `validateSchemaNode`, replacing it with the new utility function for improved code clarity and maintainability. - This refactor enhances the error handling mechanism across the workflow, ensuring consistent behavior when errors occur. Co-authored-by: [Your Name] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a few things I was concerned about, but I don't think it will be a problem to merge them.Thank you!
// Conditional edges with retry logic - each node will retry up to maxRetries times on error | ||
// If maxRetries is exceeded and error persists, workflow goes to END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://langchain-ai.github.io/langgraphjs/how-tos/node-retry-policies/
I found it a bit hard to read, so I checked if LangChain has a built-in retry system, and it seems that you can set a retryPolicy on addNode. It may be better to match this one.
Migration could possibly solve the following problems: https://github.com/liam-hq/liam/pull/2167/files#r2163037269
There is no problem to merge once to solve this issue!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, thank you! Let me think for a bit if I should recreate the pull request.🤔
I will recreate the pull request.🙏 |
Why is this change needed?
Fixed a failing test case in
workflow.test.ts:283
and implemented comprehensive retry handling across all workflow nodes to ensure robust error management.should handle schema update failure
was failing due to LangGraph channel update conflicts1. Fixed LangGraph Channel Update Issues
LastValue can only receive one value per step
errors2. Implemented Comprehensive Retry Handling
3. Resolved Node Name Mismatches
NODE_NAME
constants in node files and workflow edge definitionsWhat would you like reviewers to focus on?
Testing Verification
✅ All 13 test cases now pass
✅ Confirmation that workflow can be executed from chat
https://cloud.trigger.dev/orgs/liam-hq-5035/projects/liam-HdAt/env/preview-fix-test-error/runs/run_cmca3txthbq3727mxmhsn26j4?span=e5ccd84e09799319
What was done
🤖 Generated by PR Agent at 7e7fd8b
Detailed Changes
2 files
Rename NODE_NAME constant for consistency
Rename NODE_NAME constant for consistency
7 files
Add retry handling and rename NODE_NAME
Implement comprehensive error handling and retry logic
Add retry handling and rename NODE_NAME
Enhance error handling and rename NODE_NAME
Implement comprehensive error handling and retry logic
Add retry handling and rename NODE_NAME
Add retry handling and rename NODE_NAME
1 files
Implement unified retry logic with getNextNodeOrEnd
1 files
Fix LangGraph annotations and increase recursion limit
1 files
Re-enable failing test and update error assertions
Additional Notes