Skip to content
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

backend: re-work job table usages after v2 #5219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

uael
Copy link
Collaborator

@uael uael commented Feb 5, 2025

Important

Refactor backend job handling to improve flow status updates, error handling, and remove deprecated preview fetching methods.

  • Behavior:
    • Refactor job data fetching by removing deprecated preview fetching methods in cache.rs and worker.rs.
    • Update flow status handling in worker_flow.rs to improve error handling and processing efficiency.
    • Modify resolve_raw_values() in jobs.rs to use runnable_id instead of id and hash.
  • Database Queries:
    • Add new SQL queries in .sqlx files for fetching job and flow data.
    • Update existing queries in worker_flow.rs and worker.rs to align with new job data handling.
  • Error Handling:
    • Enhance error handling in update_flow_status_after_job_completion() in worker_flow.rs.
    • Improve logging and error messages across worker.rs and worker_flow.rs.
  • Misc:
    • Remove unused imports and code in jobs.rs and worker.rs.
    • Minor refactoring in result_processor.rs to align with new job data handling.

This description was created by Ellipsis for f8b7e22. It will automatically update as commits are pushed.

@uael uael force-pushed the uael/v2_rework_job_usages branch from 0dd408d to 79e0ac3 Compare February 6, 2025 07:54
Copy link

cloudflare-workers-and-pages bot commented Feb 6, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2e2764e
Status: ✅  Deploy successful!
Preview URL: https://22c2e30b.windmill.pages.dev
Branch Preview URL: https://uael-v2-rework-job-usages.windmill.pages.dev

View logs

@uael uael force-pushed the uael/v2_rework_job_usages branch 3 times, most recently from 0fb674b to 40375cf Compare February 7, 2025 08:35
@uael uael requested a review from alpetric February 7, 2025 08:40
@uael uael marked this pull request as ready for review February 7, 2025 08:42
@uael uael requested a review from rubenfiszel as a code owner February 7, 2025 08:42
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 40375cf in 1 minute and 41 seconds

More details
  • Looked at 1196 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 14 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_flow.rs:185
  • Draft comment:
    The update_flow_status_after_job_completion_internal function is very complex and lengthy. Consider breaking it into smaller, focused helper functions to improve readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-worker/src/worker_flow.rs:1601
  • Draft comment:
    The use of CRASH_FORCEFULLY_AT_STEP (lines 1601-1609) for testing purposes may lead to unintended panics if not carefully gated. Ensure this testing feature is disabled in production.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-worker/src/worker_flow.rs:1347
  • Draft comment:
    The eval_timeout call in compute_bool_from_expr injects several context variables into a JavaScript evaluation. Ensure that input is properly sanitized to mitigate security risks.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-worker/src/worker_flow.rs:3530
  • Draft comment:
    The nested match expressions and branch for for-loop status (starting around line 3530) make the logic hard to follow. Consider refactoring and adding comments to aid future maintainers.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-worker/src/worker_flow.rs:3860
  • Draft comment:
    There are several unwraps (or unwrap_or_else calls) and expect calls throughout the flow payload construction functions. Consider handling errors more gracefully to avoid panics in production.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-worker/src/worker_flow.rs:2960
  • Draft comment:
    The extensive raw SQL queries throughout the code appear correct but could be encapsulated or parameterized better to improve clarity and maintenance. Consider documenting expectations for each query.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. backend/windmill-worker/src/worker_flow.rs:203
  • Draft comment:
    The function update_flow_status_after_job_completion_internal is very long and deeply nested. Consider refactoring portions into smaller helper functions to improve readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. backend/windmill-worker/src/worker_flow.rs:1610
  • Draft comment:
    The helper potentially_crash_for_testing() is used for testing purposes. Ensure it is strictly enabled only in test builds to avoid accidental crashes in production.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. backend/windmill-worker/src/worker_flow.rs:3004
  • Draft comment:
    The recursive insert_iter_arg function may risk a stack overflow if many conflicting keys arise. Consider an iterative solution or impose a recursion limit.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. backend/windmill-worker/src/worker_flow.rs:1347
  • Draft comment:
    The compute_bool_from_expr function expects the evaluated expression to return exactly the strings "true" or "false". Consider parsing the result as a JSON boolean to handle more natural boolean outputs.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. backend/windmill-worker/src/worker_flow.rs:800
  • Draft comment:
    Multiple inline SQL queries perform JSONB_SET updates with similar patterns. Refactoring these into helper functions would reduce duplication and simplify maintenance.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. backend/windmill-worker/src/worker_flow.rs:3696
  • Draft comment:
    Using panic! when the iterator ('itered') is empty in next_loop_iteration may not be ideal. Returning an error would allow for graceful handling.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. backend/windmill-worker/src/worker_flow.rs:423
  • Draft comment:
    The transaction management in update_flow_status_after_job_completion_internal (and similar functions) is complex, with nested commits and reassignments. Consider refactoring to isolate transactional updates in dedicated helper routines to ease error handling.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. backend/windmill-worker/src/worker_flow.rs:1
  • Draft comment:
    Many public and complex functions in this module lack Rust doc comments. Adding documentation would aid future maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_qellmYvI0TZYUkQp


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@uael uael force-pushed the uael/v2_rework_job_usages branch from 40375cf to 068ab0b Compare February 10, 2025 07:10
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 068ab0b in 1 minute and 35 seconds

More details
  • Looked at 1196 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 15 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_flow.rs:68
  • Draft comment:
    update_flow_status_after_job_completion_internal is extremely complex with many nested match arms and inline SQL update queries. Consider refactoring into smaller, more testable helper functions.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-worker/src/worker_flow.rs:1325
  • Draft comment:
    The compute_bool_from_expr function relies on eval_timeout returning exactly 'true' or 'false'. This fragile string‐based check may lead to bugs if the evaluator ever returns a boolean in other form. Consider converting the evaluation result directly to a boolean.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-worker/src/worker_flow.rs:2410
  • Draft comment:
    Inside push_next_flow_job, the loop over subflow payloads issues individual SQL queries for each job. For large flows this could impact performance; consider batching these database writes.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-worker/src/worker_flow.rs:3750
  • Draft comment:
    There are multiple uses of unwrap/expect (eg, in payload_from_simple_module and elsewhere) that may cause panics in production. Consider propagating errors instead.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-worker/src/worker_flow.rs:3646
  • Draft comment:
    eval_timeout is used to evaluate expressions from input transforms. Ensure that the evaluated expressions are properly sandboxed to avoid arbitrary code execution vulnerabilities.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-worker/src/worker_flow.rs:3960
  • Draft comment:
    Global testing utilities like potentially_crash_for_testing() (lines 1614-1622) should be conditionally compiled under proper feature flags to avoid unintended crashes in production.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. backend/windmill-worker/src/worker_flow.rs:2150
  • Draft comment:
    Frequent SQL updates using JSONB_SET for flow status may cause performance bottlenecks under heavy load. Consider reviewing whether these updates can be batched or optimized.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. backend/windmill-worker/src/worker_flow.rs:215
  • Draft comment:
    The function 'update_flow_status_after_job_completion_internal' is extremely complex with many nested match arms and SQL updates. Consider splitting it into smaller helper functions and adding more inline documentation to improve readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. backend/windmill-worker/src/worker_flow.rs:1348
  • Draft comment:
    In 'compute_bool_from_expr', the function expects the evaluated expression to return the strings 'true' or 'false'. This is brittle; consider parsing the result into a boolean directly to avoid errors if the evaluator returns variants (e.g. capitalized or numeric).
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. backend/windmill-worker/src/worker_flow.rs:3018
  • Draft comment:
    The 'insert_iter_arg' function uses recursion to rename conflicting keys. While clever, it may benefit from additional comments explaining the expected key hierarchy and safeguards against potential infinite recursion if keys repeatedly conflict.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. backend/windmill-worker/src/worker_flow.rs:3593
  • Draft comment:
    The 'is_simple_modules' function checks many fields to determine whether the module is 'simple'. Consider adding documentation that explains the rationale behind each field check so that future maintainers understand the criteria.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. backend/windmill-worker/src/worker_flow.rs:2579
  • Draft comment:
    Within 'push_next_flow_job', a loop iterates over payloads and issues individual SQL queries (e.g., updating ping every 100 iterations). If the number of subflow jobs is high, this could be a performance bottleneck. Consider batching updates or reusing transactions where possible.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. backend/windmill-worker/src/worker_flow.rs:3646
  • Draft comment:
    The evaluator 'eval_timeout' is used in multiple contexts to compute boolean values and transform input. Ensure that the context and expressions provided are from trusted sources to avoid potential injection vulnerabilities.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. backend/windmill-worker/src/worker_flow.rs:3690
  • Draft comment:
    Several SQL queries update JSON fields in 'flow_status' using JSONB_SET. This repetition increases maintenance burden and risks inconsistencies. Consider refactoring these common update patterns into helper functions to simplify and centralize the logic.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. backend/windmill-worker/src/worker_flow.rs:2959
  • Draft comment:
    Overall, many functions in this file are very long and deeply nested. Consider breaking some of these functions into smaller, well-documented helper functions to improve readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_j9JgLBILAIIzoSIy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on f8b7e22 in 1 minute and 28 seconds

More details
  • Looked at 1067 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 15 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_flow.rs:68
  • Draft comment:
    The top-level function update_flow_status_after_job_completion is very complex. Consider splitting its logic into smaller sub-functions and adding more inline documentation.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-worker/src/worker_flow.rs:186
  • Draft comment:
    The function update_flow_status_after_job_completion_internal mixes numerous SQL queries and nested logic. Refactoring to extract helper functions and clearly label SQL operations would improve maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-worker/src/worker_flow.rs:1318
  • Draft comment:
    The helper function next_retry relies on chaining and then mapping with potential ambiguity. Consider clarifying its return type and adding comments on how the retry duration is computed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-worker/src/worker_flow.rs:1325
  • Draft comment:
    The compute_bool_from_expr function uses eval_timeout and then compares string output. Converting the evaluation result to a native boolean type (rather than a string) would be more robust and clear.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-worker/src/worker_flow.rs:3611
  • Draft comment:
    The function next_forloop_status contains complex logic that re-evaluates an iterator using eval_timeout. Consider breaking out parts of this logic into smaller helper functions to improve readability and simplify error handling.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-worker/src/worker_flow.rs:68
  • Draft comment:
    The function update_flow_status_after_job_completion is very long and complex. Consider splitting some of its logic into helper functions for better readability and testing.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. backend/windmill-worker/src/worker_flow.rs:186
  • Draft comment:
    update_flow_status_after_job_completion_internal has a huge block of logic with many nested match arms and SQL updates. Refactor into smaller, well-documented helper functions to ease maintenance.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. backend/windmill-worker/src/worker_flow.rs:3018
  • Draft comment:
    The recursive insert_iter_arg function may be prone to deep recursion if many key conflicts occur. Consider using an iterative approach to avoid potential stack overflows.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. backend/windmill-worker/src/worker_flow.rs:1348
  • Draft comment:
    In compute_bool_from_expr, the eval_timeout call returns a string that must be exactly 'true' or 'false'. This strict expectation may be brittle if the evaluated expression returns a native boolean or other truthy value.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. backend/windmill-worker/src/worker_flow.rs:3868
  • Draft comment:
    The functions raw_script_to_payload and script_to_payload contain multiple unwrap_or and expect calls. Ensure that missing data is handled gracefully rather than panicking.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. backend/windmill-worker/src/worker_flow.rs:1600
  • Draft comment:
    The use of potentially_crash_for_testing (lines ~1614–1622) introduces forced panic for testing purposes. Ensure this feature flag is disabled in production builds to avoid unintended crashes.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. backend/windmill-worker/src/worker_flow.rs:2170
  • Draft comment:
    The repeated SQL updates using JSONB_SET (for example in update_flow_status_after_job_completion_internal) are complex and error-prone if the JSON schema changes. Consider isolating these updates in helper functions or abstractions.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. backend/windmill-worker/src/worker_flow.rs:3612
  • Draft comment:
    In next_forloop_status, invoking eval_timeout repeatedly inside the loop may incur performance overhead. Consider caching results if the same expressions are evaluated multiple times.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. backend/windmill-worker/src/worker_flow.rs:3037
  • Draft comment:
    The payload_from_modules function’s logic is hard to follow. Adding more inline documentation explaining the expected cases and transformation steps would improve maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. backend/windmill-worker/src/worker_flow.rs:3934
  • Draft comment:
    In script_to_payload, consider separating the logic for fetching script metadata (via script_hash_to_tag_and_limits) from payload construction to simplify testing and error handling.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_k8cUBgNMRyl7J0fp


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@uael uael force-pushed the uael/v2_rework_job_usages branch 3 times, most recently from 8ad1da4 to 61e4b52 Compare February 11, 2025 16:50
@uael uael force-pushed the uael/v2_rework_job_usages branch from 61e4b52 to 2e2764e Compare February 11, 2025 20:57
@rubenfiszel rubenfiszel force-pushed the main branch 9 times, most recently from f09ee01 to 3d78825 Compare February 17, 2025 23:55
@rubenfiszel rubenfiszel force-pushed the main branch 2 times, most recently from 0d0ada6 to 1ef482e Compare February 18, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant