nits: warning retry #6760
nits: warning retry #6760
Conversation
dieriba
requested review from
alpetric,
hugocasa and
rubenfiszel
as code owners
|
Claude finished @dieriba's task —— View job Code Review: "nits: warning retry"Todo List:
Review SummaryThis PR enhances retry configuration validation in Windmill flows by adding both client-side and server-side validation for exponential backoff configurations. The changes prevent users from setting invalid retry parameters that could cause system issues. Code Quality Assessment ✅Backend Changes (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if seconds == 0 { | ||
| return Err(Error::BadRequest( | ||
| "Exponential backoff base (seconds) must be greater than 0. A base of 0 would cause immediate retries.".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The validation logic is inconsistent between the backend and frontend. The backend rejects only when seconds == 0, while the frontend validation and error message enforce seconds >= 1. To maintain consistency across the codebase:
- The backend check should be changed from
if seconds == 0toif seconds < 1 - The error message should match this logic
This would align with the frontend validation in validateRetryConfig() and the OpenAPI schema which specifies minimum: 1.
| if seconds == 0 { | |
| return Err(Error::BadRequest( | |
| "Exponential backoff base (seconds) must be greater than 0. A base of 0 would cause immediate retries.".to_string(), | |
| )); | |
| } | |
| if seconds < 1 { | |
| return Err(Error::BadRequest( | |
| "Exponential backoff base (seconds) must be at least 1. A value less than 1 would cause too frequent retries.".to_string(), | |
| )); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 723933c in 2 minutes and 54 seconds. Click for details.
- Reviewed
196lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/utils.ts:1628
- Draft comment:
The validateRetryConfig function only checks the exponential.seconds property. Consider also validating other fields (like attempts or multiplier) if needed, or add documentation to clarify its intended scope. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment points out a potential enhancement, it's speculative - we don't know if those other fields actually need validation. The function may be intentionally focused only on seconds validation. Without seeing the Retry type definition and understanding the requirements, we can't be sure if validating other fields is necessary. The comment also hedges with "if needed", making it unclear if this is actually required. The comment could be valid if there are actual requirements to validate those other fields. I may be missing context about whether those validations are truly needed. However, the core rule is to only keep comments that are clearly correct and require action. This comment is speculative and depends on unknown requirements. If additional validation was needed, it likely would have been included in the initial implementation. Delete the comment since it makes speculative suggestions without clear evidence that the additional validation is actually required.
2. openflow.openapi.yaml:90
- Draft comment:
The addition of "minimum: 1" for exponential.seconds is correct and enforces the desired constraint in the API spec. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. backend/windmill-api/src/flows.rs:96
- Draft comment:
Typo suggestion: consider rephrasing this error message for clarity. Currently it reads "Muting the error handler for certain flow is only available in enterprise version". Perhaps revise to "Muting the error handler for a certain flow is only available in the enterprise version" or "... for certain flows ..." depending on the intended meaning. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment suggests a valid grammatical improvement, it's a very minor change that doesn't significantly impact functionality or understanding. The current message is already clear enough. Making such minor text changes creates unnecessary churn and isn't worth the effort of a code change. The comment does point out a legitimate grammatical improvement. The current message could be slightly clearer. However, the current message is still perfectly understandable and the suggested change is extremely minor. We should focus PR comments on more substantive issues. Delete this comment as it suggests an extremely minor text change that isn't worth the effort of updating.
Workflow ID: wflow_aWh4bWkcueDHuNOO
You can customize 
| for (const module of flow.value.modules) { | ||
| const error = validateRetryConfig(module.retry) | ||
| if (error) { | ||
| throw new Error(error) |
There was a problem hiding this comment.
Consider including module identification (e.g. module id) in the error message when throwing an error from validateRetryConfig. This will help trace which module's retry config is invalid.
| throw new Error(error) | |
| throw new Error(`Module ${module.id}: ${error}`) |
| let result = $derived( | ||
| flowModule && flowStateStore?.val | ||
| ? (flowStateStore.val[flowModule.id]?.previewResult ?? NEVER_TESTED_THIS_FAR) | ||
| : NEVER_TESTED_THIS_FAR |
There was a problem hiding this comment.
Good use of derived stores to show the validation error. Consider if similar validation is needed for the constant retry settings or adding comments to clarify why only the exponential section is validated.
| const flow = cleanInputs(flowStore.val) | ||
| if (flow.value?.modules) { | ||
| for (const module of flow.value.modules) { |
There was a problem hiding this comment.
that only validate the first level, use dfs function to go through all the modules
| )); | ||
| } | ||
|
|
||
| if let Some(modules) = flow_value.get("modules").and_then(|m| m.as_array()) { |
There was a problem hiding this comment.
it only go through first level, there is likely a dfs function somewhere however I think it might be the wrong approach, you likely want to try to deserialize each module and fail at first module that can't be deserializing with giving the id + error from the deserializer. Much more generic. You can then just apply a validate function on a FlowModuleValue directly for anything post validation.
2 participants
Important
This PR adds validation for retry configurations in both backend and frontend, ensuring exponential backoff seconds are positive integers, and refactors code to use this validation.
validate_flow_value()inflows.rsto validate retry configurations and error handler muting.create_flow()andupdate_flow()inflows.rsto usevalidate_flow_value().validateRetryConfig()inutils.tsto validate retry configurations.validateRetryConfig()inFlowBuilder.svelteandFlowRetries.svelteto validate retry settings before saving.Retryschema inopenflow.openapi.yamlto enforce minimum value of 1 forexponential.seconds.This description was created by
for 723933c. You can customize this summary. It will automatically update as commits are pushed.